independent test cases #669
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds comprehensive integration-style tests for multiple AI providers (Anthropic, Azure, Bedrock, Cohere, Gemini, Groq, OpenRouter, Vertex) and expands OpenAI tests; tests exercise chat, tools, streaming, embeddings, responses endpoints, and provider key retrievals, gating execution on environment credentials. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (4 warnings, 1 inconclusive)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (25)
core/providers/anthropic_test.go (5)
1-1: Gate these as integration tests via build tag.Prepend a build tag so these don't run in default CI:
+//go:build integration + package providers
3-9: Add timeout and disable server‑side storage.Prevent hangs and avoid provider data retention.
import ( "context" + "time" "os" "testing" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() @@ Params: &schemas.ChatParameters{ Temperature: float64Ptr(0.7), MaxCompletionTokens: intPtr(10), + Store: boolPtr(false), + User: stringPtr("bifrost-integration-tests"), },Also applies to: 27-28, 39-43
30-33: Parameterize the model to reduce churn.Models change frequently; read from env with a sane default.
- Model: "claude-3-haiku-20240307", + Model: envOrDefault("ANTHROPIC_CHAT_MODEL", "claude-3-haiku-20240307"),You can define:
func envOrDefault(k, d string) string { if v := os.Getenv(k); v != "" { return v } return d }
131-146: Stream test: enforce timeout.Mirror the same ctx.WithTimeout pattern to avoid indefinite waits.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Also applies to: 147-162
205-218: Responses: future‑proof model and privacy.Parametrize model and set Store:false.
- Model: "claude-3-5-sonnet-20241022", + Model: envOrDefault("ANTHROPIC_RESPONSES_MODEL", "claude-3-5-sonnet-20241022"), @@ Params: &schemas.ResponsesParameters{ - MaxOutputTokens: intPtr(100), + MaxOutputTokens: intPtr(100), + Store: boolPtr(false), },core/providers/vertex_test.go (4)
3-10: Add timeout and disable storage; import time.Reduce flakiness and data retention.
import ( "context" "fmt" + "time" "os" "testing" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cancel() @@ Params: &schemas.ChatParameters{ Temperature: float64Ptr(0.7), MaxCompletionTokens: intPtr(10), + Store: boolPtr(false), },Also applies to: 31-31, 49-53
12-17: Skip in short mode.Avoid accidental runs with
go test -short.if credentials == "" || projectID == "" { t.Fatal("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") } +if testing.Short() { + t.Skip("skipping Vertex integration test in -short mode") +}
40-43: Parameterize model IDs; Vertex names drift.Make models configurable via env to track publisher revisions.
- Model: "claude-3-5-haiku", + Model: envOrDefault("VERTEX_CHAT_MODEL", "claude-3-5-haiku"),Apply similarly to other occurrences (e.g., "claude-3-7-sonnet@20240229").
Also applies to: 199-201, 289-292, 361-364
333-395: Stream test: enforce timeout.Mirror ctx.WithTimeout to avoid hangs if the channel isn’t closed.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cancel()core/providers/openai_test.go (3)
3-9: Add timeout; import time.Protect the test from long network stalls.
import ( "context" + "time" "os" "testing" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Also applies to: 27-27
31-33: Modernize and parameterize OpenAI models.3.5 and ada‑002 are likely deprecated; read models from env and default to current ones.
- Model: "gpt-3.5-turbo", + Model: envOrDefault("OPENAI_CHAT_MODEL", "gpt-4o-mini"), @@ - Model: "gpt-3.5-turbo-instruct", + Model: envOrDefault("OPENAI_TEXT_MODEL", "gpt-4o-mini"), @@ - Model: "text-embedding-ada-002", + Model: envOrDefault("OPENAI_EMBEDDING_MODEL", "text-embedding-3-small"),Also applies to: 248-252, 205-213
147-147: Apply timeout to stream tests.Same ctx.WithTimeout pattern for streaming.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Also applies to: 375-375
core/providers/openrouter_test.go (3)
3-10: Add timeout; import time.Avoid stalls during network issues.
import ( "context" + "time" "os" "testing" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Also applies to: 27-27
31-33: Parameterize model name.OpenRouter free model aliases change; make configurable.
- Model: "meta-llama/llama-3.2-3b-instruct:free", + Model: envOrDefault("OPENROUTER_CHAT_MODEL", "meta-llama/llama-3.2-3b-instruct:free"),Also applies to: 153-155
166-170: Stream test: add timeout.Apply the same ctx.WithTimeout pattern here.
- streamChan, bfErr := provider.ChatCompletionStream(ctx, mockPostHookRunner, key, request) + streamChan, bfErr := provider.ChatCompletionStream(ctx, mockPostHookRunner, key, request)And set ctx with timeout as suggested above (around Line 149).
Also applies to: 171-186
core/providers/cohere_test.go (4)
3-10: Add timeout; import time.Keep integration tests bounded.
import ( "context" + "time" "os" "testing" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Repeat the ctx.WithTimeout change in other tests in this file (streaming too).
Also applies to: 27-27, 241-241
32-33: Parameterize Cohere models.Use env vars to absorb future renames.
- Model: "command-r-08-2024", + Model: envOrDefault("COHERE_CHAT_MODEL", "command-r-08-2024"), @@ - Model: "embed-english-v3.0", + Model: envOrDefault("COHERE_EMBEDDING_MODEL", "embed-english-v3.0"),Also applies to: 96-97, 172-173, 246-247, 301-303
39-43: Set Store:false (privacy).Avoid persisting test conversations.
Params: &schemas.ChatParameters{ Temperature: float64Ptr(0.7), MaxCompletionTokens: intPtr(10), + Store: boolPtr(false), },
231-240: Stream test: apply timeout pattern.Mirror ctx.WithTimeout and keep chunk loop unchanged.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Also applies to: 258-278
core/providers/gemini_test.go (2)
12-14: Prefert.Skipovert.Fatalfor missing environment variables.Integration tests that require external API keys should use
t.Skiprather thant.Fatalwhen credentials are unavailable, allowing the test suite to continue with other tests.Apply this pattern consistently:
- if apiKey == "" { - t.Fatal("GEMINI_API_KEY not set") - } + if apiKey == "" { + t.Skip("GEMINI_API_KEY not set") + }Also applies to: 68-70, 136-138, 190-192
17-24: Consider extracting provider initialization to a test helper.Provider configuration is duplicated across all test functions with identical values. Extract to a helper function to improve maintainability.
Example helper:
func setupGeminiProvider(t *testing.T) *GeminiProvider { return NewGeminiProvider(&schemas.ProviderConfig{ NetworkConfig: schemas.NetworkConfig{ DefaultRequestTimeoutInSeconds: 30, }, ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ Concurrency: 10, }, }, newTestLogger()) }Also applies to: 73-80, 141-148, 195-202
core/providers/bedrock_test.go (2)
12-14: Prefert.Skipovert.Fatalfor missing environment variables.Integration tests should use
t.Skipwhen API credentials are unavailable, allowing other tests to continue execution.Apply this pattern:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }Also applies to: 77-79, 154-156, 250-252, 313-315
17-27: Consider extracting provider and key setup to test helpers.Provider initialization and key configuration are duplicated across all tests. Extracting common setup would improve maintainability.
Also applies to: 82-92, 159-169, 255-265, 318-328, 375-385
core/providers/azure_test.go (1)
18-28: Consider extracting provider and key setup to test helpers.Provider initialization and Azure-specific key configuration are duplicated across all tests, reducing maintainability.
Also applies to: 80-90, 160-170, 259-269, 340-350, 406-416, 453-463
core/providers/groq_test.go (1)
17-28: Consider extracting provider setup to a test helper.Provider initialization with BaseURL configuration is duplicated across all tests, reducing maintainability.
Also applies to: 71-82, 143-154, 201-212, 241-251
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(4 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
core/providers/anthropic_test.go (6)
core/providers/anthropic.go (1)
NewAnthropicProvider(79-117)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Anthropic(37-37)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(305-305)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/groq_test.go (5)
core/providers/groq.go (1)
NewGroqProvider(27-62)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Groq(43-43)core/schemas/transcriptions.go (1)
BifrostTranscriptionRequest(3-9)
core/providers/bedrock_test.go (6)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(305-305)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/azure_test.go (6)
core/providers/azure.go (1)
NewAzureProvider(32-56)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Azure(36-36)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/gemini_test.go (6)
core/providers/gemini.go (1)
NewGeminiProvider(35-66)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Gemini(47-47)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/vertex_test.go (4)
core/providers/vertex.go (1)
NewVertexProvider(68-83)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Vertex(40-40)
core/providers/openrouter_test.go (5)
core/providers/openrouter.go (1)
NewOpenRouterProvider(27-57)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenRouter(48-48)
core/providers/cohere_test.go (6)
core/providers/cohere.go (1)
NewCohereProvider(58-91)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Cohere(39-39)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/openai_test.go (1)
core/schemas/bifrost.go (1)
BifrostError(319-328)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
core/providers/vertex_test.go (1)
19-29: Disregard this review comment; code is valid for the project's Go version.The project targets Go 1.24 (per core/go.mod), which has supported ranging over integers since Go 1.22 (February 2024). The syntax on line 72 (
for range config.ConcurrencyAndBufferSize.Concurrency) is valid and will compile without issues.Likely an incorrect or invalid review comment.
core/providers/openai_test.go (1)
31-33: All models verified as currently recommended as of October 23, 2025.Chat API correctly uses
gpt-3.5-turbo(recommended as the most cost-effective GPT-3.5 family model).gpt-3.5-turbo-instructis the correct Completions-compatible model for legacy completions use.text-embedding-ada-002is the recommended embeddings model.core/providers/azure_test.go (1)
14-16: Good use oft.Skipfor optional integration tests.Correctly uses
t.Skipinstead oft.Fatalfor missing environment variables, allowing other tests to continue when credentials are unavailable.Also applies to: 76-78, 156-158, 255-257, 336-338, 402-404
core/providers/groq_test.go (1)
13-15: Good patterns for optional and incomplete tests.Correctly uses
t.Skipfor missing environment variables and provides clear explanation for skipping the transcription test until audio data is available.Also applies to: 67-69, 139-141, 196-199, 217-219
| request := &schemas.BifrostChatRequest{ | ||
| Provider: schemas.Anthropic, | ||
| Model: "claude-3-haiku-20240307", | ||
| Input: []schemas.ChatMessage{ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Anthropic model IDs (may have changed).
Please confirm latest model slugs for Messages and Responses as of 2025‑10‑23.
Also applies to: 88-115, 150-162, 205-218
🌐 Web query:
What are Anthropic’s current model IDs for Haiku and Sonnet (Messages API and Responses API), and which “latest” aliases should be used?
💡 Result:
- Claude Haiku (current model ID): claude-haiku-4-5-20251001 — use alias: claude-haiku-4-5. [1][2]
- Claude Sonnet (current model ID): claude-sonnet-4-5-20250929 — use alias: claude-sonnet-4-5. [1][2]
Sources: Anthropic / Claude model overview and API client docs. [1] (docs.claude.com) [2] (docs.claude.com)
Citations:
- 1: https://docs.claude.com/en/docs/models-overview?utm_source=openai
- 2: https://docs.claude.com/en/api/client-sdks?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Inspect the test file at the specified line ranges
echo "=== Lines 30-33 ==="
sed -n '30,33p' core/providers/anthropic_test.go
echo ""
echo "=== Lines 88-115 ==="
sed -n '88,115p' core/providers/anthropic_test.go
echo ""
echo "=== Lines 150-162 ==="
sed -n '150,162p' core/providers/anthropic_test.go
echo ""
echo "=== Lines 205-218 ==="
sed -n '205,218p' core/providers/anthropic_test.goLength of output: 1780
🏁 Script executed:
#!/bin/bash
# Inspect the test file at the specified line ranges
echo "=== Lines 30-33 ==="
sed -n '30,33p' core/providers/anthropic_test.go
echo ""
echo "=== Lines 88-115 ==="
sed -n '88,115p' core/providers/anthropic_test.go
echo ""
echo "=== Lines 150-162 ==="
sed -n '150,162p' core/providers/anthropic_test.go
echo ""
echo "=== Lines 205-218 ==="
sed -n '205,218p' core/providers/anthropic_test.goLength of output: 1780
Update all Anthropic model IDs to current versions (claude-haiku-4-5 and claude-sonnet-4-5).
Claude Haiku's current model ID is claude-haiku-4-5-20251001 and Claude Sonnet's current model ID is claude-sonnet-4-5-20250929. Update:
- Lines 30-33, 88-115, 150-162: Change
"claude-3-haiku-20240307"to"claude-haiku-4-5"(or full version ID) - Lines 205-218: Change
"claude-3-5-sonnet-20241022"to"claude-sonnet-4-5"(or full version ID)
🤖 Prompt for AI Agents
In core/providers/anthropic_test.go around lines 30-33, 88-115, 150-162 and
205-218, the tests use outdated Anthropic model IDs; replace occurrences of
"claude-3-haiku-20240307" with the current haiku model ID ("claude-haiku-4-5" or
the full version "claude-haiku-4-5-20251001") and replace occurrences of
"claude-3-5-sonnet-20241022" with the current sonnet model ID
("claude-sonnet-4-5" or the full version "claude-sonnet-4-5-20250929"); update
the string literals in those lines so tests reference the new model IDs
consistently.
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | ||
| } |
There was a problem hiding this comment.
Validate that tool calls actually occurred in all tool-related tests.
The three tool tests (single, multiple, and parallel) log tool call counts without asserting that any tools were invoked. Tests could pass even if tool functionality is broken.
Add validation in all tool tests:
if len(resp.Choices) == 0 {
t.Fatal("Expected at least one choice")
}
+ if len(resp.Choices[0].Message.ToolCalls) == 0 {
+ t.Fatal("Expected at least one tool call")
+ }
t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))For the parallel tool calls test specifically, consider validating multiple calls occurred:
if len(resp.Choices[0].Message.ToolCalls) < 2 {
t.Logf("Warning: Expected multiple parallel tool calls, got %d", len(resp.Choices[0].Message.ToolCalls))
}Also applies to: 243-250, 324-331
🤖 Prompt for AI Agents
In core/providers/azure_test.go around lines 144-151, 243-250 and 324-331, the
tests only log the number of tool calls but do not assert that any tools were
actually invoked; update each tool-related test to validate that resp and
resp.Choices are non-nil and assert that len(resp.Choices[0].Message.ToolCalls)
> 0 (fail the test if zero) so tool invocation failures surface, and for the
parallel tool-calls test (the 324-331 block) validate that multiple calls
occurred—either assert len(...) >= 2 or, if nondeterministic, log a warning when
fewer than 2 calls are present; ensure error messages include the actual count
for clearer diagnostics.
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | ||
| } |
There was a problem hiding this comment.
Validate that tool calls actually occurred in tool-related tests.
Both single and multiple tool tests log the tool call count without asserting that any tools were invoked. Tests could pass even if tool calling functionality is broken.
Add validation in both tests:
if len(resp.Choices) == 0 {
t.Fatal("Expected at least one choice")
}
+ if len(resp.Choices[0].Message.ToolCalls) == 0 {
+ t.Fatal("Expected at least one tool call")
+ }
t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))Also applies to: 240-247
🤖 Prompt for AI Agents
In core/providers/bedrock_test.go around lines 144-151 (and similarly at
240-247), the tests only log the number of tool calls but do not assert that any
tool calls actually occurred; update both tests to assert that
resp.Choices[0].Message.ToolCalls is non-nil and has length > 0 (e.g., use
t.Fatalf or t.Fatalf with context when nil or empty) so the test fails if no
tool was invoked; keep the existing nil and choices checks and add these
explicit assertions (and mirror the same checks in the other test block at
240-247).
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | ||
| } |
There was a problem hiding this comment.
Validate that tool calls actually occurred.
The test logs the tool call count but doesn't assert that any tools were invoked. The model might respond without calling the tool, causing the test to pass incorrectly.
Add validation after line 131:
if len(resp.Choices) == 0 {
t.Fatal("Expected at least one choice")
}
+ if len(resp.Choices[0].Message.ToolCalls) == 0 {
+ t.Fatal("Expected at least one tool call")
+ }
t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))📝 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.
| if resp == nil { | |
| t.Fatal("Expected non-nil response") | |
| } | |
| if len(resp.Choices) == 0 { | |
| t.Fatal("Expected at least one choice") | |
| } | |
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | |
| } | |
| if resp == nil { | |
| t.Fatal("Expected non-nil response") | |
| } | |
| if len(resp.Choices) == 0 { | |
| t.Fatal("Expected at least one choice") | |
| } | |
| if len(resp.Choices[0].Message.ToolCalls) == 0 { | |
| t.Fatal("Expected at least one tool call") | |
| } | |
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | |
| } |
🤖 Prompt for AI Agents
In core/providers/gemini_test.go around lines 126 to 133, the test only logs the
number of tool calls and can pass when none occurred; add a firm assertion after
line 131 that the first choice's ToolCalls slice is non-nil and has length > 0
(e.g., if resp.Choices[0].Message.ToolCalls == nil || len(...) == 0 {
t.Fatal("Expected at least one tool call") }) so the test fails when the model
did not invoke any tools.
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | ||
| } |
There was a problem hiding this comment.
Validate that tool calls actually occurred.
The test logs tool call count without asserting that any tools were invoked, allowing the test to pass even if tool functionality is broken.
Add validation:
if len(resp.Choices) == 0 {
t.Fatal("Expected at least one choice")
}
+ if len(resp.Choices[0].Message.ToolCalls) == 0 {
+ t.Fatal("Expected at least one tool call")
+ }
t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))📝 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.
| if resp == nil { | |
| t.Fatal("Expected non-nil response") | |
| } | |
| if len(resp.Choices) == 0 { | |
| t.Fatal("Expected at least one choice") | |
| } | |
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | |
| } | |
| if resp == nil { | |
| t.Fatal("Expected non-nil response") | |
| } | |
| if len(resp.Choices) == 0 { | |
| t.Fatal("Expected at least one choice") | |
| } | |
| if len(resp.Choices[0].Message.ToolCalls) == 0 { | |
| t.Fatal("Expected at least one tool call") | |
| } | |
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | |
| } |
🤖 Prompt for AI Agents
In core/providers/groq_test.go around lines 128 to 135, the test currently only
logs the number of tool calls which allows it to pass when no tools were
invoked; update the assertions to validate that
resp.Choices[0].Message.ToolCalls is non-nil and has length > 0 (i.e., assert
that at least one tool call occurred), and fail the test with a clear message if
that condition is not met; keep the existing nil/choice checks and replace or
augment the Logf with these explicit assertions.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
core/providers/openrouter_test.go (2)
3-9: Add per-test context timeouts to avoid hangs.Networked tests should bound runtime. Prefer a helper and use it in each test.
@@ -import ( - "context" +import ( + "context" + "time" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel() @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel()Optionally extract:
func testCtx() (context.Context, context.CancelFunc) { return context.WithTimeout(context.Background(), 25*time.Second) }Use in all tests in this file.
Also applies to: 27-28, 149-151
30-43: Make model/token params configurable to reduce flakiness.Allow overriding model and limits via env to cope with quota/model availability.
Example:
model := envOrDefault("OPENROUTER_TEST_MODEL", "meta-llama/llama-3.2-3b-instruct:free") maxTok := intFromEnvOrDefault("OPENROUTER_MAX_TOK", 10)Also applies to: 152-164
core/providers/groq_test.go (3)
195-205: Skip early in transcription test to avoid unnecessary setup.Move t.Skip to the top to avoid creating clients when the test is disabled.
func TestGroqTranscription(t *testing.T) { + t.Skip("Transcription test requires audio file - implement when needed") apiKey := os.Getenv("GROQ_API_KEY") - if apiKey == "" { - t.Skip("GROQ_API_KEY not set") - } + if apiKey == "" { t.Skip("GROQ_API_KEY not set") } @@ - // Note: This test requires an actual audio file. Skipping for now. - // In a real test, you would provide a base64-encoded audio file. - t.Skip("Transcription test requires audio file - implement when needed") + // In a real test, you would provide a base64-encoded audio file.Also applies to: 217-220
3-9: Add per-test timeouts.Same rationale as other providers; avoid hanging CI.
@@ -import ( - "context" +import ( + "context" + "time" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel() @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel() @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel()Also applies to: 30-33, 84-86, 156-158
94-121: Optional: assert tool-call presence when requesting tools.If the model is expected to call tools, assert len(ToolCalls) > 0 to catch regressions; otherwise keep as log to avoid flakiness.
core/providers/openai_test.go (2)
3-9: Add per-test timeouts.Guard external calls with timeouts.
@@ -import ( - "context" +import ( + "context" + "time" @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel() @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel() @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) + defer cancel()Also applies to: 27-29, 147-149, 202-204
31-43: Parameterize models via env to reduce brittleness.Older model IDs can be EOL’d. Make models overridable using env vars (OPENAI_TEST_MODEL_CHAT, OPENAI_TEST_MODEL_TEXT, OPENAI_TEST_MODEL_EMBEDDING).
Also applies to: 88-116, 150-163, 248-252
core/providers/cohere_test.go (2)
27-29: Add per-test timeouts.Prevent hangs in external calls.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()Add:
+ "time"to the import list.
Also applies to: 84-86, 153-155, 241-243, 296-298
94-121: Optional: strengthen tool-call assertions.If you expect tool invocation, assert len(ToolCalls) > 0; otherwise keep logs to avoid flakiness.
Also applies to: 170-209
core/providers/bedrock_test.go (2)
29-36: Add per-test timeouts.External AWS calls can stall; bound runtime.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel()Also add:
+ "time"to the import list.
Also applies to: 94-101, 171-178, 267-274, 330-337
312-372: Responses test: optional include/exhaustiveness.Consider asserting that at least one output text segment is non-empty to catch empty responses despite 200 OK.
found := false for _, m := range resp.Output { if m.Content != nil && m.Content.ContentStr != nil && *m.Content.ContentStr != "" { found = true; break } } if !found { t.Fatal("Expected non-empty output text") }core/providers/gemini_test.go (1)
26-27: Bound test duration with a context timeout.External calls can hang; wrap ctx with a timeout to keep tests deterministic.
+ // at top: import "time" ctx := context.Background() + // Replace with a bounded context: + // ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + // defer cancel()Also applies to: 150-151
core/providers/vertex_test.go (1)
31-38: Optional: add a context timeout for external calls.Bound runtime for reliability when upstreams stall.
- ctx := context.Background() + // ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + // defer cancel()Also add
timeimport.Also applies to: 98-105, 176-183, 273-280, 352-359, 416-423
core/providers/anthropic_test.go (2)
27-29: Optional: add context timeouts.Guard external calls with ctx timeouts for deterministic test behavior.
- ctx := context.Background() + // ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + // defer cancel()Also add
timeimport.Also applies to: 78-80, 147-149, 202-204
11-16: Short-mode guard for integration tests (optional).Skip in
go test -shortto keep unit and integration layers separate.if testing.Short() { t.Skip("skipping Anthropic integration test in -short mode") }core/providers/azure_test.go (1)
424-425: Update embedding model to current generation; use env var for deployment flexibility.text-embedding-ada-002 remains available on Azure OpenAI but is outdated. OpenAI/Microsoft recommend text-embedding-3-large for best quality or text-embedding-3-small as a cost-effective alternative. Hardcoding the model name is brittle; parameterize via env var (e.g.,
AZURE_EMB_DEPLOYMENT) to allow easy updates without code changes.Applies to lines: 424-425, 431-433
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(4 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
core/providers/azure_test.go (5)
core/providers/azure.go (1)
NewAzureProvider(32-56)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Azure(36-36)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/groq_test.go (6)
core/providers/groq.go (1)
NewGroqProvider(27-62)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Groq(43-43)core/schemas/transcriptions.go (1)
BifrostTranscriptionRequest(3-9)
core/providers/bedrock_test.go (5)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (2)
BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/gemini_test.go (5)
core/providers/gemini.go (1)
NewGeminiProvider(35-66)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (5)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Gemini(47-47)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/cohere_test.go (5)
core/providers/cohere.go (1)
NewCohereProvider(58-91)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Cohere(39-39)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/vertex_test.go (6)
core/providers/vertex.go (1)
NewVertexProvider(68-83)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Vertex(40-40)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/anthropic_test.go (6)
core/providers/anthropic.go (1)
NewAnthropicProvider(79-117)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Anthropic(37-37)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(305-305)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/openrouter_test.go (5)
core/providers/openrouter.go (1)
NewOpenRouterProvider(27-57)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenRouter(48-48)
core/providers/openai_test.go (1)
core/schemas/bifrost.go (1)
BifrostError(319-328)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (3)
core/providers/openrouter_test.go (1)
11-16: No issues found—all test helpers are properly defined and accessible.The test helpers
newTestLogger,stringPtr,intPtr,float64Ptr, andmockPostHookRunnerare all defined incore/providers/test_helpers.goand are package-visible toopenrouter_test.goand other test files in the same package. No compile errors will occur.core/providers/openai_test.go (2)
205-213: Struct rename verified and correctly applied. TextCompletionParameters correctly definesMaxTokens(line 121, core/schemas/textcompletions.go), with noMaxCompletionTokensfield present. Usage is consistent across the codebase—no build breaks or missing updates detected. The test code snippet correctly uses the field.
45-47: ****The claim that OpenAI ChatCompletion returns
errorwhile others return*schemas.BifrostErroris incorrect. All 14 providers, including OpenAI (openai.go:434), return the same signature:(*schemas.BifrostChatResponse, *schemas.BifrostError). The test code's use of the variable nameerris merely a naming convention and does not reflect the actual return type—the second return value is consistently*schemas.BifrostErroracross all providers.Likely an incorrect or invalid review comment.
| Temperature: float64Ptr(0.7), | ||
| MaxCompletionTokens: intPtr(200), | ||
| ParallelToolCalls: boolPtr(true), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove cross‑file test coupling: localize boolPtr.
This file calls boolPtr but defines it only in vertex_test.go. Define a local helper here to keep tests independent.
Add at the end of this file:
// Helper for bool pointers (keeps this test file self-contained)
func boolPtr(b bool) *bool { return &b }🤖 Prompt for AI Agents
In core/providers/azure_test.go around lines 299 to 301, this test uses boolPtr
which is defined in another test file causing cross-file coupling; add a local
helper to this file to return a *bool (e.g., at the end of
core/providers/azure_test.go) so tests are self-contained by defining: func
boolPtr(b bool) *bool { return &b }.
| apiKey := os.Getenv("BEDROCK_API_KEY") | ||
| if apiKey == "" { | ||
| t.Fatal("BEDROCK_API_KEY not set") | ||
| } |
There was a problem hiding this comment.
Use t.Skip when BEDROCK_API_KEY is unset (don’t fail CI by default).
Failing when env isn’t configured will redline CI. Skip instead, consistent with other providers.
- if apiKey == "" {
- t.Fatal("BEDROCK_API_KEY not set")
- }
+ if apiKey == "" {
+ t.Skip("BEDROCK_API_KEY not set")
+ }Apply to all occurrences in this file.
Also applies to: 77-81, 154-157, 250-253, 313-316
🤖 Prompt for AI Agents
In core/providers/bedrock_test.go around lines 12-15 (and similarly at 77-81,
154-157, 250-253, 313-316), the tests call t.Fatal when BEDROCK_API_KEY is unset
which fails CI; replace those t.Fatal checks with t.Skip calls so the tests are
skipped when the environment variable is not configured. For each occurrence,
check os.Getenv("BEDROCK_API_KEY") and if empty call t.Skip with a concise
message like "BEDROCK_API_KEY not set; skipping test" rather than failing,
preserving the rest of the test logic.
| provider := NewCohereProvider(&schemas.ProviderConfig{ | ||
| NetworkConfig: schemas.NetworkConfig{ | ||
| BaseURL: "https://api.cohere.com", | ||
| DefaultRequestTimeoutInSeconds: 60, | ||
| }, | ||
| ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ | ||
| Concurrency: 10, | ||
| }, | ||
| }, newTestLogger()) |
There was a problem hiding this comment.
🧩 Analysis chain
BaseURL mismatch with provider default; confirm desired endpoint.
Tests use https://api.cohere.com, provider default snippet shows https://api.cohere.ai. Align defaults or keep explicit override here to avoid confusion.
If .com is the intended target, consider updating the provider default.
🏁 Script executed:
#!/bin/bash
rg -nP 'NewCohereProvider' -n core/providers/cohere.go -C4
rg -nP 'BaseURL.*cohere\.(ai|com)' core/providers/cohere.goLength of output: 759
I need to check whether the BaseURL assignment at line 79 is conditional or unconditional, and review the CheckAndSetDefaults() implementation:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get extended context around line 79 to see if assignment is conditional
rg -nP '\.BaseURL\s*=' -A2 -B2 core/providers/cohere.goLength of output: 276
Now I need to verify which endpoint is the correct Cohere API endpoint:
[/web_search]
🌐 Web query:
Cohere API endpoint https://api.cohere.ai vs api.cohere.com
💡 Result:
Short answer: use api.cohere.ai — that's Cohere's current official base (including the OpenAI-compatibility path at https://api.cohere.ai/compatibility/v1). Some third‑party projects or older examples have used api.cohere.com (or include it as an alternate endpoint), but Cohere's docs and SDKs point to api.cohere.ai as the supported production base URL. [1][2][3][4]
Sources:
- Cohere docs — Chat & API usage (shows Cohere endpoints/SDK). [1][2]
- Cohere docs — Compatibility API (example base_url = https://api.cohere.ai/compatibility/v1). [3]
- Example third‑party project using https://api.cohere.com/ as an endpoint in its config (not official Cohere docs). [4]
Update test to use official Cohere API endpoint.
The test uses https://api.cohere.com, but Cohere's docs and SDKs point to api.cohere.ai as the supported production base URL. While the code's conditional default at line 78 (if config.NetworkConfig.BaseURL == "") correctly preserves your explicit override, the endpoint itself is incorrect. Change the test's BaseURL to https://api.cohere.ai to align with the provider default and official Cohere documentation.
🤖 Prompt for AI Agents
In core/providers/cohere_test.go around lines 17 to 25, the test hardcodes the
Cohere BaseURL as "https://api.cohere.com" which is incorrect; update the
ProviderConfig.NetworkConfig.BaseURL value to "https://api.cohere.ai" so the
test uses the official Cohere API endpoint and aligns with the provider default
and documentation.
| provider, err := NewGroqProvider(&schemas.ProviderConfig{ | ||
| NetworkConfig: schemas.NetworkConfig{ | ||
| BaseURL: "https://api.groq.com/openai", | ||
| DefaultRequestTimeoutInSeconds: 30, | ||
| }, | ||
| ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ | ||
| Concurrency: 10, | ||
| }, | ||
| }, newTestLogger()) |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm connection pooling field usage in provider.
Provider sets MaxConnsPerHost from BufferSize, while tests set Concurrency. This may lead to unbounded or unintended pooling.
Consider aligning to Concurrency if that is the intended knob.
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'NewGroqProvider\s*\(' core/providers/groq.go
rg -nP 'MaxConnsPerHost\s*:\s*config\.ConcurrencyAndBufferSize\.(BufferSize|Concurrency)' core/providers/groq.goLength of output: 398
🏁 Script executed:
#!/bin/bash
# Get full NewGroqProvider function implementation
sed -n '25,50p' core/providers/groq.goLength of output: 1107
🏁 Script executed:
#!/bin/bash
# Find ConcurrencyAndBufferSize struct definition and defaults
rg -nP -A5 'type\s+ConcurrencyAndBufferSize\s+struct'
# Find CheckAndSetDefaults implementation
rg -nP -A10 'func.*CheckAndSetDefaults' core/providers/groq.go
# Check if CheckAndSetDefaults is on config itself
rg -nP -A10 'CheckAndSetDefaults.*ProviderConfig'Length of output: 508
🏁 Script executed:
#!/bin/bash
# Find DefaultConcurrencyAndBufferSize
rg -nP -A2 'DefaultConcurrencyAndBufferSize' core/schemas/provider.go
# Find CheckAndSetDefaults method on ProviderConfig
rg -nP -A15 '^\s*func\s*\(\s*config\s*\*ProviderConfig\s*\)\s*CheckAndSetDefaults' core/schemas/provider.goLength of output: 866
🏁 Script executed:
#!/bin/bash
# Find DefaultConcurrency and DefaultBufferSize values
rg -nP '(DefaultConcurrency|DefaultBufferSize)\s*=' core/schemas/provider.go | head -10Length of output: 83
Confirm MaxConnsPerHost should use Concurrency field, not BufferSize.
The schema documents that Concurrency is "used as the initial pool size for provider responses" (core/schemas/provider.go:54), but NewGroqProvider sets MaxConnsPerHost from BufferSize instead (core/providers/groq.go:33). The test sets Concurrency: 10, which has no effect; MaxConnsPerHost becomes DefaultBufferSize (5000) instead. Update NewGroqProvider line 33 to use config.ConcurrencyAndBufferSize.Concurrency, or correct the test to set BufferSize if that's the intended field.
🤖 Prompt for AI Agents
In core/providers/groq.go around line 33, MaxConnsPerHost is being set from
BufferSize but should use the Concurrency field per the schema; update that
assignment to use config.ConcurrencyAndBufferSize.Concurrency (or convert to the
proper int type) so the provider’s connection pool respects the configured
Concurrency value.
| credentials := os.Getenv("VERTEX_CREDENTIALS") | ||
| projectID := os.Getenv("VERTEX_PROJECT_ID") | ||
| if credentials == "" || projectID == "" { | ||
| t.Fatal("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") | ||
| } |
There was a problem hiding this comment.
Skip when Vertex env is missing (don’t fail CI).
These are integration tests; use t.Skip to keep them independent from secrets.
- if credentials == "" || projectID == "" {
- t.Fatal("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set")
- }
+ if credentials == "" || projectID == "" {
+ t.Skip("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set")
+ }Apply to all six tests listed above.
Also applies to: 80-84, 158-162, 255-259, 334-338, 398-402
🤖 Prompt for AI Agents
In core/providers/vertex_test.go around lines 13-17 (and similarly apply the
same change at lines 80-84, 158-162, 255-259, 334-338, 398-402): the test
currently calls t.Fatal when VERTEX_CREDENTIALS or VERTEX_PROJECT_ID are missing
which fails CI; change this to check the env vars and call t.Skipf (or t.Skip)
with a brief message when either variable is empty so the integration tests are
skipped instead of failing CI, and apply the same replacement in each of the
listed line ranges.
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
bff252b to
9f9801b
Compare
Pratham-Mishra04
force-pushed
the
10-22-independent_test_cases
branch
from
05d3725 to
78945a5
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
9f9801b to
3c90f16
Compare
Pratham-Mishra04
force-pushed
the
10-22-independent_test_cases
branch
2 times, most recently
from
35eea86 to
7486561
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
3c90f16 to
691f3c0
Compare
Pratham-Mishra04
force-pushed
the
10-22-independent_test_cases
branch
from
7486561 to
76994d0
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
691f3c0 to
7c005f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (15)
core/providers/bedrock_test.go (7)
12-15: Use t.Skip instead of t.Fatal for missing environment variable.This issue was flagged in a previous review. When the environment variable is not set, the test should skip rather than fail, preventing CI failures when credentials are unavailable.
Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
77-81: Use t.Skip instead of t.Fatal for missing environment variable.This issue was flagged in a previous review. The test should skip when credentials are unavailable rather than fail.
Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
144-151: Assert that tool calls actually occurred.This issue was flagged in a previous review. The test only logs the tool call count but does not verify that any tools were invoked. The test could pass even if tool calling functionality is broken.
Apply this diff:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
154-157: Use t.Skip instead of t.Fatal for missing environment variable.This issue was flagged in a previous review. The test should skip when credentials are unavailable.
Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
240-247: Assert that tool calls actually occurred.This issue was flagged in a previous review. The test only logs the tool call count without verifying that any tools were invoked.
Apply this diff:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
250-253: Use t.Skip instead of t.Fatal for missing environment variable.This issue was flagged in a previous review. The test should skip when credentials are unavailable.
Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
313-316: Use t.Skip instead of t.Fatal for missing environment variable.This issue was flagged in a previous review. The test should skip when credentials are unavailable.
Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }core/providers/vertex_test.go (3)
13-17: This issue has been flagged before.The test should use
t.Skip()instead oft.Fatal()when environment variables are missing to avoid failing CI.Also applies to: 80-84, 158-162, 255-259, 334-338, 398-402
42-42: This issue has been flagged before.Invalid Vertex AI model ID format. Should be
"claude-3-5-haiku@20241022"with the proper @yyyymmdd suffix.Also applies to: 116, 291, 363
201-201: This issue has been flagged before.Invalid Vertex AI model ID. Should be
"claude-3-7-sonnet@20250219"(correct timestamp).core/providers/anthropic_test.go (4)
30-33: Model ID is outdated (previously flagged).As noted in the previous review,
claude-3-haiku-20240307should be updated to the current model IDclaude-haiku-4-5(or the full versionclaude-haiku-4-5-20251001).
88-115: Model ID is outdated (previously flagged).As noted in the previous review,
claude-3-haiku-20240307should be updated to the current model IDclaude-haiku-4-5.
150-162: Model ID is outdated (previously flagged).As noted in the previous review,
claude-3-haiku-20240307should be updated to the current model IDclaude-haiku-4-5.
205-218: Model ID is outdated (previously flagged).As noted in the previous review,
claude-3-5-sonnet-20241022should be updated to the current model IDclaude-sonnet-4-5(or the full versionclaude-sonnet-4-5-20250929).core/providers/gemini_test.go (1)
129-133: Assert that at least one tool call occurred.Log-only can pass when no tool was invoked; add a hard assertion.
@@ - t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) + if resp.Choices[0].Message.ToolCalls == nil || len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } + t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
🧹 Nitpick comments (4)
core/providers/anthropic_test.go (1)
17-25: Extract provider initialization to a helper function.The provider initialization is duplicated across all five tests. Consider extracting this to a shared helper function to improve maintainability and reduce duplication.
Example helper:
func newTestAnthropicProvider() *AnthropicProvider { return NewAnthropicProvider(&schemas.ProviderConfig{ NetworkConfig: schemas.NetworkConfig{ BaseURL: "https://api.anthropic.com", DefaultRequestTimeoutInSeconds: 30, }, ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ Concurrency: 10, }, }, newTestLogger()) }Also applies to: 68-76, 137-145, 192-200, 235-242
core/providers/gemini_test.go (3)
207-211: Use the current Gemini Embedding model.Switch from legacy "text-embedding-004" to stable "gemini-embedding-001". It’s the recommended embeddings model in the Gemini API; "text-embedding-004" is listed as legacy. (ai.google.dev)
@@ - Model: "text-embedding-004", + Model: "gemini-embedding-001",
45-53: Optionally skip on quota/permission/auth errors to keep tests green for external issues.Treat 429/403/invalid-key style errors as skips rather than failures to reduce flakiness in CI.
@@ - if bfErr != nil { + if bfErr != nil { + // Optional: downgrade common external errors to skip + // Requires: import "strings" + if bfErr.Error != nil { + msg := strings.ToLower(bfErr.Error.Message) + if strings.Contains(msg, "quota") || strings.Contains(msg, "rate") || + strings.Contains(msg, "permission") || strings.Contains(msg, "invalid api key") { + t.Skipf("Skipping due to external error: %s", bfErr.Error.Message) + } + } if bfErr.Error != nil { if bfErr.Error.Error != nil { t.Fatalf("ChatCompletion failed - error: %v", bfErr.Error.Error) } t.Fatalf("ChatCompletion failed - message: %s", bfErr.Error.Message) } t.Fatalf("ChatCompletion failed: %v", bfErr) } @@ - if bfErr != nil { + if bfErr != nil { + if bfErr.Error != nil { + msg := strings.ToLower(bfErr.Error.Message) + if strings.Contains(msg, "quota") || strings.Contains(msg, "rate") || + strings.Contains(msg, "permission") || strings.Contains(msg, "invalid api key") { + t.Skipf("Skipping due to external error: %s", bfErr.Error.Message) + } + } t.Fatalf("ChatCompletionStream failed: %v", bfErr) } @@ - if bfErr != nil { + if bfErr != nil { + if bfErr.Error != nil { + msg := strings.ToLower(bfErr.Error.Message) + if strings.Contains(msg, "quota") || strings.Contains(msg, "rate") || + strings.Contains(msg, "permission") || strings.Contains(msg, "invalid api key") { + t.Skipf("Skipping due to external error: %s", bfErr.Error.Message) + } + } t.Fatalf("Embedding failed: %v", bfErr) }Also add:
@@ -import ( +import ( "context" + "strings" "os" "testing"Also applies to: 168-170, 213-216
101-118: Make tool invocation deterministic (optional).If supported by your provider adapter, set ToolChoice to require the tool (or strengthen the prompt) so the test consistently triggers a function call. This reduces model variance and flakes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(4 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/openai_test.go
- core/providers/azure_test.go
- core/providers/cohere_test.go
- core/providers/groq_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
core/providers/bedrock_test.go (5)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(309-309)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/gemini_test.go (6)
core/providers/gemini.go (1)
NewGeminiProvider(35-66)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Gemini(47-47)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/vertex_test.go (6)
core/providers/vertex.go (1)
NewVertexProvider(68-83)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Vertex(40-40)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/openrouter_test.go (5)
core/providers/openrouter.go (1)
NewOpenRouterProvider(27-57)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenRouter(48-48)
core/providers/anthropic_test.go (6)
core/providers/anthropic.go (1)
NewAnthropicProvider(79-117)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Anthropic(37-37)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(309-309)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (16)
core/providers/bedrock_test.go (1)
374-391: LGTM!This test correctly validates the provider key without requiring external credentials. The logic is sound and the test is appropriately scoped.
core/providers/openrouter_test.go (2)
133-186: Good streaming validation pattern.The test correctly validates streaming behavior by checking for errors in each chunk and ensuring at least one chunk is received.
188-202: LGTM!The provider key test correctly validates that
GetProviderKey()returns the expectedschemas.OpenRoutervalue.core/providers/vertex_test.go (9)
19-29: LGTM! Clean provider initialization.The provider configuration with explicit timeouts and concurrency settings is well-structured.
55-77: LGTM! Thorough error handling and response validation.The nested error checking and comprehensive response assertions are appropriate for integration tests.
107-141: LGTM! Well-structured tool definition.The tool schema with properties, description, and required fields follows best practices for function calling.
185-238: LGTM! Clean multiple tools pattern.The test properly exercises multiple tool definitions with distinct parameters, validating the provider's ability to handle complex tool scenarios.
298-317: LGTM! Appropriate parallel tool calls test.The test correctly enables
ParallelToolCallsand uses a prompt designed to exercise parallel invocations.
375-395: LGTM! Solid streaming test pattern.The stream handling correctly iterates the channel, checks for nil chunks, validates errors, and verifies output.
425-446: LGTM! Comprehensive embedding test.The test validates the embedding response structure and verifies non-empty vectors. The model ID
text-embedding-004is appropriate for Vertex AI.
448-465: LGTM! Simple and effective unit test.The provider key verification test is appropriately scoped and doesn't require external credentials.
467-470: LGTM! Standard Go pointer helper.Clean utility function for creating bool pointers, consistent with Go idioms for optional parameters.
core/providers/anthropic_test.go (2)
234-248: LGTM!This unit test is properly isolated, doesn't require external dependencies, and correctly validates the provider key.
164-164: No action required—mockPostHookRunneris properly defined.The helper is defined in
core/providers/test_helpers.go(lines 37-40) and is accessible to all test files in thecore/providerspackage, includinganthropic_test.go. The code is correct as written.Likely an incorrect or invalid review comment.
core/providers/gemini_test.go (2)
230-244: LGTM: provider key test.Correctly verifies provider identity and base URL handling.
150-187: No changes required—mockPostHookRunner is properly defined and linked.The symbol is defined in
core/providers/test_helpers.go(lines 37-40) within the same package and correctly referenced in the test. The code will compile without issues.
| apiKey := os.Getenv("GEMINI_API_KEY") | ||
| if apiKey == "" { | ||
| t.Fatal("GEMINI_API_KEY not set") | ||
| } |
There was a problem hiding this comment.
Skip when GEMINI_API_KEY is absent (don’t fail CI).
Make these tests independent by skipping instead of failing if the key isn’t set.
@@
- if apiKey == "" {
- t.Fatal("GEMINI_API_KEY not set")
- }
+ if apiKey == "" {
+ t.Skip("GEMINI_API_KEY not set")
+ }
@@
- if apiKey == "" {
- t.Fatal("GEMINI_API_KEY not set")
- }
+ if apiKey == "" {
+ t.Skip("GEMINI_API_KEY not set")
+ }
@@
- if apiKey == "" {
- t.Fatal("GEMINI_API_KEY not set")
- }
+ if apiKey == "" {
+ t.Skip("GEMINI_API_KEY not set")
+ }
@@
- if apiKey == "" {
- t.Fatal("GEMINI_API_KEY not set")
- }
+ if apiKey == "" {
+ t.Skip("GEMINI_API_KEY not set")
+ }Also applies to: 68-71, 136-139, 190-193
🤖 Prompt for AI Agents
In core/providers/gemini_test.go around lines 12-15 (and similarly at 68-71,
136-139, 190-193), the tests call t.Fatal when GEMINI_API_KEY is missing which
fails CI; replace those checks to call t.Skipf("GEMINI_API_KEY not set: skipping
test") (or t.Skip) so the tests are skipped when the environment variable is
absent; update each of the four locations to use the skip path and keep the same
env lookup logic.
| Model: "gemini-pro", | ||
| Input: []schemas.ChatMessage{ |
There was a problem hiding this comment.
Update deprecated model id "gemini-pro" to a current stable model.
Use a stable Gemini 2.5 model to avoid flakiness and ensure API compatibility. For cost/speed, prefer gemini-2.5-flash; for heavier reasoning, gemini-2.5-pro. (ai.google.dev)
@@
- Model: "gemini-pro",
+ Model: "gemini-2.5-flash",
@@
- Model: "gemini-pro",
+ Model: "gemini-2.5-flash",
@@
- Model: "gemini-pro",
+ Model: "gemini-2.5-flash",If you specifically need advanced reasoning, swap "gemini-2.5-flash" for "gemini-2.5-pro" instead. (ai.google.dev)
Also applies to: 94-95, 155-156
🤖 Prompt for AI Agents
In core/providers/gemini_test.go around lines 31-32 (and also update occurrences
at lines 94-95 and 155-156), replace the deprecated model id "gemini-pro" with a
current stable Gemini 2.5 model id; choose "gemini-2.5-flash" for
cost/speed-sensitive tests or "gemini-2.5-pro" for advanced reasoning tests, and
update the Model string values accordingly so tests use the supported model
names.
| provider := NewOpenRouterProvider(&schemas.ProviderConfig{ | ||
| NetworkConfig: schemas.NetworkConfig{ | ||
| BaseURL: "https://openrouter.ai/api", | ||
| DefaultRequestTimeoutInSeconds: 30, | ||
| }, | ||
| ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ | ||
| Concurrency: 10, | ||
| }, | ||
| }, newTestLogger()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract repeated provider setup into a helper function.
The provider configuration is duplicated across all four tests. According to the relevant code snippets, NewOpenRouterProvider already sets the default BaseURL to "https://openrouter.ai/api" and default timeout to 30 seconds, so these explicit values are redundant.
Create a shared helper function:
func newTestOpenRouterProvider() *OpenRouterProvider {
return NewOpenRouterProvider(&schemas.ProviderConfig{
ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{
Concurrency: 10,
},
}, newTestLogger())
}Then replace all provider instantiations with:
- provider := NewOpenRouterProvider(&schemas.ProviderConfig{
- NetworkConfig: schemas.NetworkConfig{
- BaseURL: "https://openrouter.ai/api",
- DefaultRequestTimeoutInSeconds: 30,
- },
- ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{
- Concurrency: 10,
- },
- }, newTestLogger())
+ provider := newTestOpenRouterProvider()Also applies to: 70-78, 139-147, 189-196
🤖 Prompt for AI Agents
In core/providers/openrouter_test.go around lines 17-25 (also apply edits at
70-78, 139-147, 189-196), the OpenRouter provider setup is duplicated and
redundantly specifies default BaseURL and timeout; extract a helper function
newTestOpenRouterProvider that calls NewOpenRouterProvider with only the
ConcurrencyAndBufferSize{Concurrency:10} and newTestLogger(), then replace each
direct provider instantiation in those test ranges with a call to
newTestOpenRouterProvider(), removing the explicit BaseURL and
DefaultRequestTimeoutInSeconds fields.
76994d0 to
840f43e
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
7c005f3 to
0d05428
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (16)
core/providers/bedrock_test.go (7)
12-15: Use t.Skip when BEDROCK_API_KEY is unset (previously flagged).This issue was already raised in a previous review. Using
t.Fatalwhen the environment variable is unset will fail CI. Tests should skip gracefully when credentials are not configured.
78-80: Use t.Skip when BEDROCK_API_KEY is unset (previously flagged).This issue was already raised in a previous review. Replace
t.Fatalwitht.Skipto allow tests to pass when credentials are unavailable.
144-151: Validate that tool calls actually occurred (previously flagged).This issue was already raised in a previous review. The test logs the tool call count but doesn't assert that any tools were invoked. The test could pass even if tool calling is broken.
155-157: Use t.Skip when BEDROCK_API_KEY is unset (previously flagged).This issue was already raised in a previous review. Replace
t.Fatalwitht.Skipfor graceful test skipping when credentials are unavailable.
240-247: Validate that tool calls actually occurred (previously flagged).This issue was already raised in a previous review. Add an assertion to verify that
resp.Choices[0].Message.ToolCallshas at least one element.
251-253: Use t.Skip when BEDROCK_API_KEY is unset (previously flagged).This issue was already raised in a previous review. Replace
t.Fatalwitht.Skipto avoid failing CI when the environment variable is not configured.
314-316: Use t.Skip when BEDROCK_API_KEY is unset (previously flagged).This issue was already raised in a previous review. Use
t.Skipinstead oft.Fatalto enable graceful skipping when credentials are absent.core/providers/openrouter_test.go (1)
62-131: Unreachable code already flagged.The unreachable API key check (lines 65-68) after the unconditional skip has been noted in past comments. Since the test is skipped due to free model limitations, consider whether this test should remain in the codebase or be updated when a paid model is available for testing.
core/providers/anthropic_test.go (4)
30-33: Update Anthropic model ID to current version.The model ID
"claude-3-haiku-20240307"is outdated. Use"claude-haiku-4-5"or the full version"claude-haiku-4-5-20251001"instead.
88-90: Update Anthropic model ID to current version.The model ID
"claude-3-haiku-20240307"is outdated. Use"claude-haiku-4-5"or the full version"claude-haiku-4-5-20251001"instead.
150-152: Update Anthropic model ID to current version.The model ID
"claude-3-haiku-20240307"is outdated. Use"claude-haiku-4-5"or the full version"claude-haiku-4-5-20251001"instead.
205-208: Update Anthropic model ID to current version.The model ID
"claude-3-5-sonnet-20241022"is outdated. Use"claude-sonnet-4-5"or the full version"claude-sonnet-4-5-20250929"instead.core/providers/vertex_test.go (3)
15-17: Uset.Skipinstead oft.Fatalfor missing environment variables.This issue was previously flagged. Integration tests should skip gracefully when credentials are unavailable rather than failing CI.
Apply this diff:
if credentials == "" || projectID == "" { - t.Fatal("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") + t.Skip("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") }Also applies to lines 82-84, 160-162, 257-259, 336-338, 400-402.
42-42: Invalid Vertex AI model ID; must include@YYYYMMDDsuffix.This issue was previously flagged. Vertex AI requires full model IDs with timestamp suffixes.
Apply this diff:
- Model: "claude-3-5-haiku", + Model: "claude-3-5-haiku@20241022",Also applies to lines 116, 291, 363.
201-201: Invalid timestamp in Vertex AI model ID.This issue was previously flagged. The correct model ID for Claude 3.7 Sonnet on Vertex is
claude-3-7-sonnet@20250219, not@20240229.Apply this diff:
- Model: "claude-3-7-sonnet@20240229", + Model: "claude-3-7-sonnet@20250219",core/providers/cohere_test.go (1)
19-21: Use the official Cohere base URL (or rely on provider default).Tests hardcode https://api.cohere.com while the provider default is https://api.cohere.ai. Align to the official endpoint or drop the override and let defaults apply.
Apply this diff to switch to api.cohere.ai:
- BaseURL: "https://api.cohere.com", + BaseURL: "https://api.cohere.ai",Occurrences to update in this file:
- Lines 19, 76, 145, 233, 288, 325.
Also applies to: 76-78, 145-147, 233-235, 288-290, 325-325
🧹 Nitpick comments (6)
core/providers/bedrock_test.go (1)
17-24: Consider extracting common test setup to reduce duplication.The provider initialization logic is repeated identically across five tests. Consider extracting it to a helper function:
func setupBedrockProvider(t *testing.T) *BedrockProvider { provider, err := NewBedrockProvider(&schemas.ProviderConfig{ NetworkConfig: schemas.NetworkConfig{ DefaultRequestTimeoutInSeconds: 60, }, ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ Concurrency: 10, }, }, newTestLogger()) if err != nil { t.Fatalf("Failed to create Bedrock provider: %v", err) } return provider }Similarly, the region (
"us-east-1") and model ("anthropic.claude-3-haiku-20240307-v1:0") could be extracted as package-level test constants for easier maintenance.Also applies to: 82-89, 159-166, 255-262, 318-325
core/providers/anthropic_test.go (1)
117-129: Add assertion to verify tool calls were generated.The test only logs the tool call count but doesn't assert that any tool calls were actually made. Given the prompt asks about weather and a
get_weathertool is provided, the model should invoke it.Apply this diff to add the assertion:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))core/providers/cohere_test.go (4)
3-9: Bound test runtime with context timeouts.Guard against hangs and keep CI predictable by using context.WithTimeout in each test.
import ( "context" "os" "testing" + "time" "github.com/maximhq/bifrost/core/schemas" ) @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() key := schemas.Key{Value: apiKey} @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() key := schemas.Key{Value: apiKey} @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() key := schemas.Key{Value: apiKey} @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 45*time.Second) // a bit more for streaming + defer cancel() key := schemas.Key{Value: apiKey} @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() key := schemas.Key{Value: apiKey}Also applies to: 27-28, 84-86, 153-155, 241-243, 296-298
253-256: Cap streaming output to reduce flakiness.Limit tokens in the streaming test to keep runs fast and predictable.
Params: &schemas.ChatParameters{ - Temperature: float64Ptr(0.7), + Temperature: float64Ptr(0.7), + MaxCompletionTokens: intPtr(32), },
11-16: Optionally skip in -short mode for cleaner local runs.Add a short-mode gate so
go test -shortbypasses networked tests. Keeps developer loops fast.apiKey := os.Getenv("COHERE_API_KEY") if apiKey == "" { t.Skip("COHERE_API_KEY not set") } + if testing.Short() { + t.Skip("skipping integration test in -short mode") + }Apply similarly in each test.
Also applies to: 68-73, 137-142, 225-230, 280-285
45-54: Unify Bifrost error handling via a tiny helper (optional).The first test unwraps nested errors; others print bfErr directly. Consider a small helper to standardize failure output.
Example:
func failOnBifrostError(t *testing.T, where string, e *schemas.BifrostError) { t.Helper() if e == nil { return } if e.Error != nil { if e.Error.Error != nil { t.Fatalf("%s: %v", where, e.Error.Error) } t.Fatalf("%s: %s", where, e.Error.Message) } t.Fatalf("%s: %v", where, e) }Then replace sites with: failOnBifrostError(t, "ChatCompletion", bfErr)
Also applies to: 123-126, 211-214, 259-261, 306-308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(4 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/groq_test.go
- core/providers/gemini_test.go
- core/providers/azure_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
core/providers/openai_test.go (1)
core/schemas/bifrost.go (1)
BifrostError(319-328)
core/providers/vertex_test.go (6)
core/providers/vertex.go (1)
NewVertexProvider(68-83)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Vertex(40-40)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/anthropic_test.go (5)
core/providers/anthropic.go (1)
NewAnthropicProvider(79-117)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Anthropic(37-37)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(309-309)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/openrouter_test.go (5)
core/providers/openrouter.go (1)
NewOpenRouterProvider(27-57)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenRouter(48-48)
core/providers/cohere_test.go (6)
core/providers/cohere.go (1)
NewCohereProvider(58-91)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Cohere(39-39)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/bedrock_test.go (6)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (2)
BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (13)
core/providers/bedrock_test.go (1)
374-391: LGTM!This unit test correctly validates the provider key without requiring external credentials. The implementation is clean and straightforward.
core/providers/openrouter_test.go (4)
1-9: LGTM!Standard package declaration and necessary imports for integration tests.
11-60: Test logic is correct.The test appropriately validates basic chat completion functionality with proper environment-based gating and reasonable assertions. The provider setup duplication has already been flagged in past comments.
133-186: LGTM!Streaming test properly handles nil chunks and errors in the stream. The chunk counting validation ensures the stream is functional. Provider setup duplication already noted in past comments.
188-202: LGTM!Simple validation test for provider key that doesn't require external API access. Logic is correct.
core/providers/anthropic_test.go (2)
234-248: LGTM!The test correctly verifies that the provider returns the expected key identifier.
164-164: Remove this review comment—the identifier is properly defined.The
mockPostHookRunnerfunction is defined incore/providers/test_helpers.goand is available to all test files in the same package, includinganthropic_test.go. No compilation errors will occur.Likely an incorrect or invalid review comment.
core/providers/vertex_test.go (2)
448-465: LGTM!The provider key test is well-structured and correctly verifies that the provider returns the expected key.
468-470: LGTM!Clean helper function for creating bool pointers.
core/providers/openai_test.go (3)
59-59: LGTM: Safer logging format.The change from
%sto%vis an improvement. The%vformat handles nil pointers gracefully and provides better debugging output for complex types.Also applies to: 226-226
265-267: I need to search the codebase directly for theEmbeddingStructdefinition to confirm its structure and verify thatEmbeddingArrayis a valid field:This review comment is incorrect. The concern is based on a false assumption about the
Embeddingfield's type.From the codebase, the
EmbeddingDatastruct definesEmbeddingasEmbeddingStruct, which is a value type field, not a pointer. In Go, there is no way to set a struct value to nil (though you could set a pointer to a struct to nil). Therefore, theEmbeddingfield cannot be nil; it will always be a zero-valuedEmbeddingStructat minimum.Accessing
.EmbeddingArrayon a value-type struct field will not cause a nil pointer panic. The code at lines 265-267 is safe and correct.Likely an incorrect or invalid review comment.
171-176: Code changes are correct.The nil chunk check (lines 171-173) is good defensive programming for channel iteration. The
chunk.BifrostErrorreference (line 174) correctly accesses the unified error type inBifrostStream.Note: Anthropic provider still uses
chunk.Errorbecause it handles provider-specificAnthropicStreamEventtypes that convert to Bifrost types—this is not an incomplete migration but separate provider implementations.core/providers/cohere_test.go (1)
258-261: Helper symbols confirmed to exist—no action needed.All test helpers (
mockPostHookRunner,stringPtr,intPtr,float64Ptr) are properly defined incore/providers/test_helpers.gowithin the same package. The code at lines 258–261 will compile without errors.
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
0d05428 to
83263a8
Compare
Pratham-Mishra04
force-pushed
the
10-22-independent_test_cases
branch
from
840f43e to
2cc8d2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
core/providers/openrouter_test.go (2)
17-25: Extract repeated provider setup into a helper function.The provider configuration is duplicated across all tests. Per the provider implementation, the BaseURL and timeout default values are already set, making these explicit values redundant.
Create a shared helper:
func newTestOpenRouterProvider() *OpenRouterProvider { return NewOpenRouterProvider(&schemas.ProviderConfig{ ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ Concurrency: 10, }, }, newTestLogger()) }Then replace all provider instantiations:
- provider := NewOpenRouterProvider(&schemas.ProviderConfig{ - NetworkConfig: schemas.NetworkConfig{ - BaseURL: "https://openrouter.ai/api", - DefaultRequestTimeoutInSeconds: 30, - }, - ConcurrencyAndBufferSize: schemas.ConcurrencyAndBufferSize{ - Concurrency: 10, - }, - }, newTestLogger()) + provider := newTestOpenRouterProvider()Also applies to: 70-78, 139-147, 189-196
62-68: Move unconditional skip after API key check.The unconditional t.Skip at line 63 makes the API key check at lines 65-68 unreachable. While past reviews noted this was addressed, the issue remains in the current code.
Reorder the checks:
func TestOpenRouterChatCompletionWithTools(t *testing.T) { - t.Skip("Free model doesn't support tools") - apiKey := os.Getenv("OPENROUTER_API_KEY") if apiKey == "" { t.Skip("OPENROUTER_API_KEY not set") } + + t.Skip("Free model doesn't support tools")core/providers/groq_test.go (2)
123-135: Validate that tool calls actually occurred.The test logs tool call count without asserting that any tools were invoked.
Add validation:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
17-25: Note: Concurrency setting may not affect connection pooling as intended.Tests set Concurrency: 10, but the Groq provider implementation uses BufferSize for MaxConnsPerHost (core/providers/groq.go:33), not Concurrency. This means the connection pool size may not match the test's intent. Consider verifying the provider implementation aligns with schema documentation stating "Concurrency is used as the initial pool size."
Also applies to: 71-79, 143-151, 201-209, 241-248
core/providers/gemini_test.go (3)
121-133: Validate that tool calls actually occurred.The test logs tool call count without asserting that any tools were invoked.
Add validation:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
11-15: Use t.Skip when API key is missing (don't fail CI).Tests use t.Fatal when GEMINI_API_KEY is unset, which fails CI. Change to t.Skip to allow graceful skipping when credentials are unavailable.
Apply to all tests:
- if apiKey == "" { - t.Fatal("GEMINI_API_KEY not set") - } + if apiKey == "" { + t.Skip("GEMINI_API_KEY not set") + }Also applies to: lines 68-71, 136-139, 190-193
Also applies to: 67-71, 135-139, 189-193
29-32: Update deprecated model ID to current Gemini 2.5 model.The model ID "gemini-pro" is deprecated. Use a current stable Gemini 2.5 model to avoid potential API compatibility issues.
Update all occurrences:
- Model: "gemini-pro", + Model: "gemini-2.5-flash",Use "gemini-2.5-pro" if advanced reasoning is required instead.
Also applies to: lines 94, 155
Also applies to: 92-95, 153-156
core/providers/bedrock_test.go (2)
139-151: Validate that tool calls actually occurred in both tool tests.Both tool-related tests log tool call counts without asserting that any tools were invoked.
Add validation in both tests:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))Also applies to: 235-247
11-15: Use t.Skip when credentials are missing (don't fail CI).Tests use t.Fatal when BEDROCK_API_KEY is unset, which fails CI. Change to t.Skip to allow graceful skipping when credentials are unavailable.
Apply to all tests:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }Also applies to: lines 77-81, 154-157, 250-253, 313-316
Also applies to: 76-81, 153-157, 249-253, 312-316
core/providers/azure_test.go (2)
139-151: Validate that tool calls actually occurred in all three tool tests.The three tool-related tests log tool call counts without asserting that any tools were invoked.
Add validation in all tests:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))For the parallel tool calls test (lines 319-331), consider also validating multiple calls occurred:
if len(resp.Choices[0].Message.ToolCalls) < 2 { t.Logf("Warning: Expected multiple parallel tool calls, got %d", len(resp.Choices[0].Message.ToolCalls)) }Also applies to: 238-250, 319-331
298-302: Define boolPtr locally to avoid cross-file test coupling.Line 301 uses boolPtr which is defined only in vertex_test.go, creating unwanted coupling between test files.
Add at the end of this file:
// Helper for bool pointers func boolPtr(b bool) *bool { return &b }core/providers/vertex_test.go (2)
12-17: Use t.Skip when credentials are missing (don't fail CI).Tests use t.Fatal when VERTEX_CREDENTIALS or VERTEX_PROJECT_ID are unset, which fails CI. Change to t.Skip to allow tests to be skipped gracefully when credentials are unavailable.
Apply this pattern to all tests in this file:
- if credentials == "" || projectID == "" { - t.Fatal("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") - } + if credentials == "" || projectID == "" { + t.Skip("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") + }Also applies to: lines 80-84, 158-162, 255-259, 334-338, 398-402
40-42: Fix invalid Vertex AI model IDs.Vertex AI requires the full model ID with @yyyymmdd suffix. The bare alias "claude-3-5-haiku" is not valid.
Apply these corrections:
- Model: "claude-3-5-haiku", + Model: "claude-3-5-haiku@20241022",Also applies to: lines 116, 291, 363
For line 201, the timestamp is also incorrect:
- Model: "claude-3-7-sonnet@20240229", + Model: "claude-3-7-sonnet@20250219",core/providers/cohere_test.go (1)
17-25: Update to official Cohere API endpoint.Tests use
https://api.cohere.com, but Cohere's official API endpoint ishttps://api.cohere.aiper their documentation and SDK defaults.Update all occurrences:
- BaseURL: "https://api.cohere.com", + BaseURL: "https://api.cohere.ai",Also applies to: lines 76, 145, 233, 288, 325
Also applies to: 74-82, 143-151, 231-239, 286-294, 323-330
🧹 Nitpick comments (3)
core/providers/openai_test.go (1)
117-129: Validate that tool calls actually occurred.The test logs the tool call count but doesn't assert that any tools were invoked, allowing the test to pass even if tool functionality is broken.
Add validation after line 127:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))core/providers/vertex_test.go (1)
143-155: Validate that tool calls actually occurred in all tool tests.The three tool-related tests log tool call counts without asserting that any tools were invoked, allowing tests to pass even if tool functionality is broken.
Add validation in each test after the choices check:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))For the parallel tool calls test (lines 319-331), consider also validating that multiple calls occurred:
if len(resp.Choices[0].Message.ToolCalls) < 2 { t.Logf("Warning: Expected multiple parallel tool calls, got %d", len(resp.Choices[0].Message.ToolCalls)) }Also applies to: 240-252, 319-331
core/providers/cohere_test.go (1)
123-135: Validate that tool calls actually occurred.Both tool-related tests log tool call counts without asserting that any tools were invoked.
Add validation in both tests:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))Also applies to: 211-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(1 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/anthropic_test.go
🧰 Additional context used
🧬 Code graph analysis (8)
core/providers/groq_test.go (6)
core/providers/groq.go (1)
NewGroqProvider(27-62)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Groq(43-43)core/schemas/transcriptions.go (1)
BifrostTranscriptionRequest(3-9)
core/providers/bedrock_test.go (5)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(309-309)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/vertex_test.go (6)
core/providers/vertex.go (1)
NewVertexProvider(68-83)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Vertex(40-40)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/gemini_test.go (6)
core/providers/gemini.go (1)
NewGeminiProvider(36-67)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Gemini(47-47)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/openrouter_test.go (5)
core/providers/openrouter.go (1)
NewOpenRouterProvider(27-57)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenRouter(48-48)
core/providers/azure_test.go (6)
core/providers/azure.go (1)
NewAzureProvider(32-56)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Azure(36-36)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/cohere_test.go (6)
core/providers/cohere.go (1)
NewCohereProvider(58-91)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Cohere(39-39)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/openai_test.go (6)
core/providers/openai.go (1)
NewOpenAIProvider(37-73)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenAI(35-35)core/schemas/textcompletions.go (3)
BifrostTextCompletionRequest(10-16)TextCompletionInput(74-77)TextCompletionParameters(115-135)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (16)
core/providers/groq_test.go (2)
22-25: Concurrency knob likely ineffective for Groq provider.Provider sets MaxConnsPerHost from BufferSize, but test sets Concurrency; either set BufferSize here or fix provider to use Concurrency.
Run to confirm in repo:
#!/bin/bash rg -nP 'MaxConnsPerHost.*ConcurrencyAndBufferSize\.(BufferSize|Concurrency)' core/providers/groq.go -n
128-135: Assert that at least one tool call occurred (don’t just log).Without this, the test passes even if tools are broken.
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))core/providers/azure_test.go (2)
299-301: Remove cross-file coupling: define boolPtr locally.Keeps this test self-contained.
func TestAzureGetProviderKey(t *testing.T) { ... } + +// Helper for bool pointers (keeps this file self-contained) +func boolPtr(b bool) *bool { return &b }
144-151: Assert tool calls > 0 in all tool tests.Fail when zero to catch regressions.
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))Also applies to: 243-250, 324-331
core/providers/vertex_test.go (1)
13-17: Skip when Vertex env is missing (keep tests independent).Replace t.Fatal with t.Skip in all guards.
- if credentials == "" || projectID == "" { - t.Fatal("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") - } + if credentials == "" || projectID == "" { + t.Skip("VERTEX_CREDENTIALS or VERTEX_PROJECT_ID not set") + }Apply at all listed ranges.
Also applies to: 80-84, 158-162, 255-259, 334-338, 398-402
core/providers/gemini_test.go (3)
12-15: Skip when GEMINI_API_KEY is missing.Use t.Skip to keep tests independent from secrets.
- if apiKey == "" { - t.Fatal("GEMINI_API_KEY not set") - } + if apiKey == "" { + t.Skip("GEMINI_API_KEY not set") + }Apply at all listed ranges.
Also applies to: 68-71, 136-139, 190-193
31-32: Update deprecated "gemini-pro" to a current stable model.Prefer gemini-2.5-flash (or gemini-2.5-pro if needed). (ai.google.dev)
- Model: "gemini-pro", + Model: "gemini-2.5-flash", @@ - Model: "gemini-pro", + Model: "gemini-2.5-flash", @@ - Model: "gemini-pro", + Model: "gemini-2.5-flash",Also applies to: 94-95, 155-156
126-133: Assert that at least one tool call occurred.Don’t just log; fail when zero.
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))core/providers/cohere_test.go (1)
17-21: Use the official Cohere API base URL.Switch from api.cohere.com to api.cohere.ai to align with docs. (docs.cohere.com)
- BaseURL: "https://api.cohere.com", + BaseURL: "https://api.cohere.ai",Apply at all listed ranges.
Also applies to: 74-78, 143-147, 231-235, 323-326
core/providers/bedrock_test.go (7)
12-15: Use t.Skip for missing credentials (past review not addressed).This issue was flagged in a previous review. Tests should skip when credentials are unavailable, not fail CI.
Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
77-81: Use t.Skip for missing credentials (past review not addressed).Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
144-151: Validate that tool calls occurred (past review not addressed).The test logs but doesn't assert that tools were invoked. A broken tool-calling implementation would pass.
Apply this diff:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
154-157: Use t.Skip for missing credentials (past review not addressed).Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
240-247: Validate that tool calls occurred (past review not addressed).Apply this diff:
if len(resp.Choices) == 0 { t.Fatal("Expected at least one choice") } + if len(resp.Choices[0].Message.ToolCalls) == 0 { + t.Fatal("Expected at least one tool call") + } t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))
250-253: Use t.Skip for missing credentials (past review not addressed).Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
313-316: Use t.Skip for missing credentials (past review not addressed).Apply this diff:
- if apiKey == "" { - t.Fatal("BEDROCK_API_KEY not set") - } + if apiKey == "" { + t.Skip("BEDROCK_API_KEY not set") + }
🧹 Nitpick comments (1)
core/providers/gemini_test.go (1)
207-211: Consider using the newer embedding model."text-embedding-004" is legacy; switch to "gemini-embedding-001" where possible. (ai.google.dev)
- Model: "text-embedding-004", + Model: "gemini-embedding-001",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(1 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/openrouter_test.go
- core/providers/anthropic_test.go
- core/providers/openai_test.go
🧰 Additional context used
🧬 Code graph analysis (6)
core/providers/azure_test.go (6)
core/providers/azure.go (1)
NewAzureProvider(32-56)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Azure(36-36)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/bedrock_test.go (6)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (3)
ResponsesInputMessageRoleUser(309-309)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
core/providers/vertex_test.go (6)
core/providers/vertex.go (1)
NewVertexProvider(68-83)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Vertex(40-40)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/groq_test.go (6)
core/providers/groq.go (1)
NewGroqProvider(27-62)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Groq(43-43)core/schemas/transcriptions.go (1)
BifrostTranscriptionRequest(3-9)
core/providers/cohere_test.go (6)
core/providers/cohere.go (1)
NewCohereProvider(58-91)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Cohere(39-39)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/gemini_test.go (6)
core/providers/gemini.go (1)
NewGeminiProvider(36-67)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Gemini(47-47)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | ||
| } |
There was a problem hiding this comment.
Assert tool calls > 0.
Strengthen tool tests so they fail if no tool invocation occurred.
if len(resp.Choices) == 0 {
t.Fatal("Expected at least one choice")
}
+ if len(resp.Choices[0].Message.ToolCalls) == 0 {
+ t.Fatal("Expected at least one tool call")
+ }
t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))Also applies to: 216-223
🤖 Prompt for AI Agents
In core/providers/cohere_test.go around lines 128 to 135, the test only logs the
number of tool calls but does not fail when none occurred; update the test to
assert that resp.Choices[0].Message.ToolCalls has length > 0 and call
t.Fatalf/t.Fatal with a clear message if it is zero. Apply the same change to
the similar block around lines 216 to 223 so both tests fail when no tool
invocation occurred.
| Provider: schemas.Vertex, | ||
| Model: "claude-3-5-haiku", | ||
| Input: []schemas.ChatMessage{ |
There was a problem hiding this comment.
🧩 Analysis chain
Fix invalid Vertex model IDs (require @Version suffix).
Bare aliases like "claude-3-5-haiku" are not valid on Vertex; use full IDs. "claude-3-7-sonnet@20250219" and "claude-3-5-haiku@20241022" are documented.
Sources: Request predictions with Claude models (model list and “must use @ suffix”), and Claude 3.7 Sonnet model card. (cloud.google.com)
- Model: "claude-3-5-haiku",
+ Model: "claude-3-5-haiku@20241022",
@@
- Model: "claude-3-5-haiku",
+ Model: "claude-3-5-haiku@20241022",
@@
- Model: "claude-3-7-sonnet@20240229",
+ Model: "claude-3-7-sonnet@20250219",
@@
- Model: "claude-3-5-haiku",
+ Model: "claude-3-5-haiku@20241022",
@@
- Model: "claude-3-5-haiku",
+ Model: "claude-3-5-haiku@20241022",Also applies to: 114-117, 199-202, 290-292, 362-364
🏁 Script executed:
#!/bin/bash
# Verify no bare 'claude-3-5-haiku"' or wrong 3.7 Sonnet remain
rg -n 'claude-3-5-haiku"' core/providers/vertex_test.go
rg -n 'claude-3-7-sonnet@' core/providers/vertex_test.goLength of output: 188
Fix invalid Vertex model IDs (require @Version suffix).
Vertex AI requires Claude model IDs to include a version suffix via @ format. Four instances of bare "claude-3-5-haiku" and one outdated "claude-3-7-sonnet@20240229" remain unfixed:
- Lines 42, 116, 291, 363: Replace
"claude-3-5-haiku"with"claude-3-5-haiku@20241022" - Line 201: Replace
"claude-3-7-sonnet@20240229"with"claude-3-7-sonnet@20250219"
🤖 Prompt for AI Agents
In core/providers/vertex_test.go around lines 42, 116, 201, 291, and 363, update
the invalid/outdated Vertex model IDs: replace "claude-3-5-haiku" at lines 42,
116, 291, and 363 with "claude-3-5-haiku@20241022", and replace
"claude-3-7-sonnet@20240229" at line 201 with "claude-3-7-sonnet@20250219".
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) | ||
| } |
There was a problem hiding this comment.
Assert tool calls > 0.
Strengthen tool tests to fail when no tool was invoked.
if len(resp.Choices) == 0 {
t.Fatal("Expected at least one choice")
}
+ if len(resp.Choices[0].Message.ToolCalls) == 0 {
+ t.Fatal("Expected at least one tool call")
+ }
t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls))Also applies to: 245-252, 324-331
🤖 Prompt for AI Agents
In core/providers/vertex_test.go around lines 148-155 (also apply same change at
245-252 and 324-331), the test only checks that resp and resp.Choices exist but
doesn't assert that a tool was actually invoked; update the assertions to
require resp.Choices[0].Message.ToolCalls length > 0 (fail the test if zero) and
include a clear t.Fatalf message indicating no tool calls were recorded; ensure
nil checks remain and perform the tool-calls length check after those nil/len
validations.
2cc8d2a to
1e10215
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
2 times, most recently
from
fc1b40e to
9063e88
Compare
Pratham-Mishra04
force-pushed
the
10-22-independent_test_cases
branch
from
1e10215 to
b3b5a90
Compare
akshaydeo
changed the base branch from
10-22-chore_tests_fixes
to
graphite-base/669
akshaydeo
force-pushed
the
10-22-independent_test_cases
branch
from
b3b5a90 to
6b00e06
Compare
akshaydeo
force-pushed
the
10-22-independent_test_cases
branch
from
6b00e06 to
f58a347
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
core/providers/openrouter_test.go (1)
17-25: Extract repeated provider setup into a helper function.Also applies to: 70-78, 139-147, 189-196
core/providers/gemini_test.go (3)
12-15: Skip tests when API key is absent (don't fail CI).Also applies to: 68-71, 136-139, 190-193
126-133: Validate that tool calls actually occurred.
31-32: Update deprecated model id "gemini-pro" to a current stable model.Also applies to: 94-95, 155-156
core/providers/cohere_test.go (2)
19-19: Update test to use official Cohere API endpoint.Also applies to: 76-76, 145-145, 233-233, 288-288, 325-325
134-134: Assert tool calls > 0.Also applies to: 222-222
core/providers/bedrock_test.go (2)
12-15: Use t.Skip when BEDROCK_API_KEY is unset (don't fail CI by default).Also applies to: 77-80, 154-157, 250-253, 313-316
150-150: Validate that tool calls actually occurred in tool-related tests.Also applies to: 246-246
🧹 Nitpick comments (4)
core/providers/gemini_test.go (1)
17-24: Extract repeated provider configuration into a helper.Provider setup is duplicated across all four tests. Consider extracting to a
newTestGeminiProvider()helper function to reduce duplication and improve maintainability.Also applies to: 73-80, 141-148, 195-202
core/providers/openai_test.go (1)
17-25: Extract repeated provider setup into a helper.Provider configuration is duplicated across all six tests. Consider extracting to a
newTestOpenAIProvider()helper to reduce duplication.Also applies to: 68-76, 137-145, 192-200, 235-243, 272-279
core/providers/cohere_test.go (1)
17-25: Extract repeated provider setup into a helper.Provider configuration is duplicated across all six tests. Extract to a
newTestCohereProvider()helper to reduce duplication.Also applies to: 74-82, 143-151, 231-239, 286-294, 323-330
core/providers/bedrock_test.go (1)
17-27: Extract repeated provider setup into a helper.Provider instantiation is duplicated across all six tests. Consider extracting to a
newTestBedrockProvider()helper for consistency and maintainability.Also applies to: 82-92, 159-169, 255-265, 318-328, 375-385
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/providers/anthropic_test.go(1 hunks)core/providers/azure_test.go(1 hunks)core/providers/bedrock_test.go(1 hunks)core/providers/cohere_test.go(1 hunks)core/providers/gemini_test.go(1 hunks)core/providers/groq_test.go(1 hunks)core/providers/openai_test.go(1 hunks)core/providers/openrouter_test.go(1 hunks)core/providers/vertex_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- core/providers/vertex_test.go
- core/providers/azure_test.go
- core/providers/groq_test.go
- core/providers/anthropic_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
core/providers/openai_test.go (7)
core/providers/openai.go (1)
NewOpenAIProvider(37-73)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenAI(35-35)core/schemas/textcompletions.go (3)
BifrostTextCompletionRequest(10-16)TextCompletionInput(74-77)TextCompletionParameters(115-135)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/gemini_test.go (6)
core/providers/gemini.go (1)
NewGeminiProvider(36-67)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Gemini(47-47)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/openrouter_test.go (5)
core/providers/openrouter.go (1)
NewOpenRouterProvider(27-57)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
OpenRouter(48-48)
core/providers/cohere_test.go (6)
core/providers/cohere.go (1)
NewCohereProvider(58-91)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Cohere(39-39)core/schemas/embedding.go (2)
BifrostEmbeddingRequest(9-15)EmbeddingInput(26-31)
core/providers/bedrock_test.go (6)
core/providers/bedrock.go (1)
NewBedrockProvider(62-79)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)core/schemas/account.go (1)
Key(8-17)core/schemas/chatcompletions.go (6)
BifrostChatRequest(11-17)ChatParameters(144-173)ChatTool(191-195)ChatToolTypeFunction(186-186)ChatToolFunction(198-203)ToolFunctionParameters(206-213)core/schemas/bifrost.go (1)
Bedrock(38-38)core/schemas/responses.go (2)
BifrostResponsesRequest(32-38)ResponsesParameters(74-101)
| if resp == nil { | ||
| t.Fatal("Expected non-nil response") | ||
| } | ||
| if len(resp.Choices) == 0 { | ||
| t.Fatal("Expected at least one choice") | ||
| } | ||
| t.Logf("Tool calls: %d", len(resp.Choices[0].Message.ToolCalls)) |
There was a problem hiding this comment.
Assert that tool calls actually occurred.
The test logs tool call count but doesn't verify any tools were invoked. Add an assertion after line 127 to fail if len(resp.Choices[0].Message.ToolCalls) == 0.
🤖 Prompt for AI Agents
In core/providers/openai_test.go around lines 122 to 128, the test currently
only logs the number of tool calls; add an assertion immediately after the log
to fail the test when no tool calls occurred by checking if
len(resp.Choices[0].Message.ToolCalls) == 0 and calling t.Fatal (or t.Fatalf)
with an explanatory message so the test fails when no tool invocations were
made.
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines