feat: tests fixes and improvements #661
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis release bumps versions across core (1.2.12→1.2.13), framework (1.1.14→1.1.15), and plugins, refactors Gemini provider types (CustomContent→Content), enhances request logging with type information, adds latency tracking throughout test scenarios, extends the mocker plugin with responses request support and per-request skip flags, and expands test infrastructure with fallback configurations and environment-controlled parallelization. Changes
Sequence Diagram(s)sequenceDiagram
participant Request as Incoming Request
participant PreHook as Mocker PreHook
participant CtxCheck as Context Check
participant RuleMatch as Rule Matching
participant Generate as Response Generation
participant ShortCircuit as Short-Circuit
Request->>PreHook: BifrostRequest + Context
PreHook->>CtxCheck: Read skip-mocker context key
alt skip-mocker = true
CtxCheck-->>PreHook: Skip enabled
PreHook-->>Request: Pass through (no mock)
else skip-mocker not set or false
CtxCheck-->>PreHook: Continue processing
PreHook->>RuleMatch: Match compiled rules<br/>against conditions
alt Rule matches
RuleMatch-->>PreHook: Rule found
PreHook->>PreHook: Check rule.Probability<br/>Random >= Probability?
alt Probability passes
PreHook->>Generate: Generate response<br/>by request type
alt ChatCompletionRequest
Generate-->>Generate: Build ChatResponse<br/>with Model, Usage, Latency
else ResponsesRequest
Generate-->>Generate: Build ResponsesResponse<br/>with Output, Usage, Latency
end
Generate-->>ShortCircuit: Call generateSuccessShortCircuit<br/>(startTime included)
ShortCircuit-->>Request: Return mocked response
else Probability fails
PreHook-->>Request: Skip activation
end
else No rule matches
RuleMatch-->>PreHook: Use default behavior
PreHook-->>Request: Pass through or apply default
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Rationale: The diff spans a heterogeneous mix of changes across provider implementations, test infrastructure, and public API surfaces. Key complexity drivers include: (1) Gemini type system refactor affecting seven schema files with public API changes, (2) mocker plugin logic refactoring supporting multiple request types with conditional flows, (3) extensive test file modifications with latency validation, fallback field mappings, and parallelization guards (many files but repetitive patterns reduce per-file cost), (4) Azure embeddings configuration and API version handling, (5) UI validation logic for wildcard origins with regex-like patterns. While many changes follow consistent patterns (test parallelization, version bumps), the Gemini schema consolidation and mocker plugin enhancements require careful line-by-line review to verify type safety and control flow correctness. Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (91)
Comment |
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
2 times, most recently
from
27411d9 to
b98ee58
Compare
Pratham-Mishra04
force-pushed
the
10-22-docs_documentation_fixes_and_updates
branch
from
058dd7c to
c0b9a3d
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
b98ee58 to
0b28240
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/core-providers/scenarios/simple_chat.go (1)
69-77: Add missing Fallbacks field to BifrostChatRequest in simple_chat.go.The BifrostChatRequest (lines 70-76) lacks the
Fallbacks: testConfig.Fallbacks,field present in the BifrostResponsesRequest at line 104. This inconsistency contradicts the pattern in automatic_function_calling.go, multiple_tool_calls.go, tool_calls.go, image_url.go, image_base64.go, end_to_end_tool_calling.go, complete_end_to_end.go, chat_completion_stream.go, and multi_turn_conversation.go, where both APIs receive Fallbacks configuration.plugins/mocker/main.go (1)
857-860: Critical: Nil pointer dereference when overriding model for ResponsesRequest.Lines 857-860 attempt to override the model without checking the request type:
if content.Model != nil { mockResponse.ChatResponse.Model = *content.Model }If
req.RequestType == schemas.ResponsesRequest, thenmockResponse.ChatResponsewill be nil (onlymockResponse.ResponsesResponseis set), causing a panic when attempting to access.Model.This will crash the application when a ResponsesRequest triggers a mock rule with a model override.
Apply this diff to fix the bug:
- // Override model if specified - if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + // Override model if specified + if content.Model != nil { + if req.RequestType == schemas.ChatCompletionRequest && mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } else if req.RequestType == schemas.ResponsesRequest && mockResponse.ResponsesResponse != nil { + // ResponsesResponse doesn't have a Model field at the top level + // The model is part of the ExtraFields which is already set + } + }Alternatively, if ResponsesResponse should support model override, you'll need to add that field to the schema or handle it differently.
🧹 Nitpick comments (1)
plugins/mocker/main.go (1)
604-604: Good refactor: Using slices.Contains for cleaner code.Replacing the explicit loop with
slices.Containsmakes the code more idiomatic and concise. This is the correct approach for Go 1.21+.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (49)
core/bifrost.go(1 hunks)core/changelog.md(1 hunks)core/schemas/bifrost.go(1 hunks)core/version(1 hunks)docs/features/plugins/mocker.mdx(5 hunks)framework/changelog.md(1 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/main.go(6 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(0 hunks)plugins/semanticcache/plugin_responses_test.go(3 hunks)plugins/semanticcache/test_utils.go(4 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/anthropic_test.go(2 hunks)tests/core-providers/azure_test.go(2 hunks)tests/core-providers/bedrock_test.go(2 hunks)tests/core-providers/cerebras_test.go(2 hunks)tests/core-providers/cohere_test.go(3 hunks)tests/core-providers/config/account.go(3 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(2 hunks)tests/core-providers/mistral_test.go(2 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(2 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(2 hunks)tests/core-providers/scenarios/automatic_function_calling.go(2 hunks)tests/core-providers/scenarios/embedding.go(1 hunks)tests/core-providers/scenarios/multiple_tool_calls.go(2 hunks)tests/core-providers/scenarios/reasoning.go(1 hunks)tests/core-providers/scenarios/simple_chat.go(1 hunks)tests/core-providers/sgl_test.go(2 hunks)tests/core-providers/vertex_test.go(2 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_edge_cases_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
plugins/semanticcache/test_utils.go (4)
plugins/mocker/main.go (7)
MockRule(77-85)Conditions(89-94)Response(98-104)ResponseTypeSuccess(28-28)SuccessResponse(108-115)MockerConfig(68-73)Init(156-202)plugins/semanticcache/main.go (2)
Plugin(137-144)Init(263-327)core/bifrost.go (2)
Bifrost(34-51)Init(80-194)core/schemas/bifrost.go (1)
BifrostConfig(21-29)
plugins/mocker/main.go (3)
core/schemas/bifrost.go (6)
BifrostContextKey(96-96)RequestType(79-79)ChatCompletionRequest(84-84)ResponsesRequest(86-86)BifrostResponse(213-223)BifrostResponseExtraFields(251-259)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)ResponsesInputMessageRoleAssistant(304-304)ResponsesMessageTypeMessage(263-263)
tests/core-providers/bedrock_test.go (1)
core/schemas/bifrost.go (2)
Fallback(114-117)Bedrock(38-38)
tests/core-providers/config/account.go (3)
core/schemas/account.go (1)
AzureKeyConfig(21-25)ui/lib/types/config.ts (1)
AzureKeyConfig(20-24)core/utils.go (1)
Ptr(12-14)
⏰ 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). (21)
- 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
- 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 (54)
plugins/governance/changelog.md (1)
4-4: Changelog entry correctly documents version updates.The entry appropriately documents the version bumps (core 1.2.13 and framework 1.1.15) consistent with the PR objectives and follows the established changelog pattern.
tests/core-providers/groq_test.go (2)
4-4: LGTM!The
osimport is correctly added to support the environment guard.
13-15: LGTM!The environment guard is properly implemented using a standard Go testing pattern. This prevents test failures when API credentials are not configured and provides a clear skip message.
tests/core-providers/cohere_test.go (1)
4-4: LGTM! Good practice for credential-gated tests.The environment variable check follows the repository-wide pattern of skipping provider tests when API keys are unavailable, preventing test failures in environments without credentials.
Also applies to: 13-15
plugins/telemetry/version (1)
1-1: LGTM! Version bump is consistent.The patch version increment from 1.3.14 to 1.3.15 aligns with the coordinated release across plugins.
plugins/maxim/version (1)
1-1: LGTM! Version bump is appropriate.The patch version increment from 1.4.14 to 1.4.15 is consistent with the coordinated release.
plugins/semanticcache/version (1)
1-1: LGTM! Version bump is consistent.The patch version increment from 1.3.14 to 1.3.15 aligns with the coordinated plugin release.
plugins/logging/version (1)
1-1: LGTM! Version bump is appropriate.The patch version increment from 1.3.14 to 1.3.15 is consistent with the coordinated plugin release.
plugins/otel/version (1)
1-1: LGTM! Version bump is appropriate.The patch version increment from 1.0.14 to 1.0.15 aligns with the coordinated release.
plugins/jsonparser/changelog.md (1)
4-4: LGTM! Changelog entry is accurate.The version update correctly references core 1.2.13 and framework 1.1.15, consistent with the coordinated release.
plugins/logging/changelog.md (1)
4-4: LGTM! Changelog entry is accurate.The version update correctly references core 1.2.13 and framework 1.1.15, consistent with the coordinated release.
transports/version (1)
1-1: LGTM! Version bump aligned with coordinated release.The version increment from 1.3.9 to 1.3.10 is part of the broader version update wave across the repository.
plugins/jsonparser/version (1)
1-1: LGTM! Version bump consistent with release.The version increment to 1.3.15 aligns with the coordinated version updates across plugins and core components.
plugins/semanticcache/changelog.md (1)
4-5: LGTM! Changelog accurately documents version and test updates.The changelog entries properly reflect the version bumps and the integration of the mocker plugin into test infrastructure.
tests/core-providers/openrouter_test.go (1)
14-14: LGTM! Improved skip message clarity.The updated skip message is more explicit and consistent with other test files in the PR.
tests/core-providers/openai_test.go (1)
13-15: LGTM! Environment guard improves test robustness.The API key check ensures tests skip gracefully when credentials are unavailable, preventing false failures in environments without OpenAI access. This pattern is consistently applied across provider tests.
transports/changelog.md (1)
4-4: LGTM! Changelog documents version dependencies.The entry appropriately records the core and framework version updates for this release.
tests/core-providers/mistral_test.go (1)
13-15: LGTM! Environment guard follows established pattern.The MISTRAL_API_KEY check ensures graceful test skipping when credentials are unavailable, consistent with the environment guard pattern applied across other provider tests.
plugins/otel/changelog.md (1)
4-4: LGTM! Changelog reflects coordinated version update.The entry appropriately documents the version bumps for core and framework dependencies.
tests/core-providers/anthropic_test.go (1)
13-15: LGTM! Clean environment-based test guard.This follows best practices by skipping tests when the required API key is unavailable, allowing the test suite to run in environments without provider credentials.
tests/core-providers/parasail_test.go (1)
13-15: LGTM! Consistent test guard pattern.tests/core-providers/sgl_test.go (1)
13-15: LGTM! Consistent environment guard for SGL tests.core/schemas/bifrost.go (1)
235-236: LGTM! Completes extra-fields support for embedding responses.This addition follows the existing pattern and enables
EmbeddingResponseto participate in the same extra-fields extraction flow as other response types. The implementation is consistent with the surrounding cases.core/bifrost.go (1)
1232-1236: LGTM! More precise error logging.The refinement adds a defensive check for
primaryErr.Errorand logs the specific error message when available, improving debuggability. The fallback to generic formatting preserves the original behavior.tests/core-providers/gemini_test.go (1)
14-14: LGTM! Consistent skip message formatting.The updated wording aligns with the skip messages used in other provider tests throughout this PR.
tests/core-providers/cerebras_test.go (1)
13-15: LGTM! Consistent environment-based test guard.tests/core-providers/scenarios/embedding.go (1)
99-109: The proposed fix is based on an incorrect understanding of the Go type system and should not be applied.After examining the code and type definitions, I found:
response.Data[0]nil check is redundant: The conditionresponseDataLength > 0(line 101) already guaranteesresponse.Data[0]exists, sinceresponseDataLength = len(response.Data).
response.Data[0].Embeddingnil check is unnecessary: Looking at the type definitions (core/schemas/embedding.go),EmbeddingData.Embeddingis of typeEmbeddingStruct, which is a value type, not a pointer. It can never be nil. The fields within it (EmbeddingArray,Embedding2DArray) can be nil, but the struct itself is always accessible.The real logical concern: If
response.Data[0].Embedding.Embedding2DArrayhas a different length thanresponse.Data, the loop at line 109 could access out-of-bounds indices. However, this appears to be a non-issue in practice because the API contract maintains a 1-to-1 correspondence betweenDataentries and embeddings.The
getEmbeddingVectorfunction already handles bothEmbeddingArrayandEmbedding2DArrayformats transparently, so the code works as intended.Likely an incorrect or invalid review comment.
tests/core-providers/vertex_test.go (1)
13-15: LGTM! Environment guard prevents test failures when credentials are missing.The guard correctly skips tests when neither VERTEX_API_KEY nor the VERTEX_PROJECT_ID+VERTEX_CREDENTIALS pair is available, aligning with the pattern used by other provider tests.
framework/version (1)
1-1: LGTM! Version bump aligns with the release.tests/core-providers/azure_test.go (1)
13-15: LGTM! Clean environment guard implementation.The guard appropriately skips Azure tests when credentials are unavailable, consistent with other provider test patterns.
plugins/mocker/version (1)
1-1: LGTM! Version bump reflects mocker plugin enhancements.core/version (1)
1-1: LGTM! Version bump reflects core improvements.plugins/telemetry/changelog.md (1)
4-4: LGTM! Changelog accurately documents version updates.plugins/maxim/changelog.md (1)
4-4: LGTM! Changelog accurately documents version updates.framework/changelog.md (1)
4-4: LGTM! Changelog accurately documents core version update.plugins/mocker/changelog.md (1)
4-6: LGTM! Changelog entries clearly document the changes.The changelog appropriately documents the version updates and new features (ResponsesRequest support and skip-mocker context key).
tests/core-providers/ollama_test.go (1)
13-15: LGTM! Environment guard follows established pattern.The early skip when
OLLAMA_BASE_URLis not set prevents unnecessary test setup and provides clear feedback. This matches the pattern used in other provider tests.tests/core-providers/scenarios/reasoning.go (1)
49-49: LGTM! Fallbacks propagation is consistent with other scenarios.This change enables fallback behavior for the Responses API reasoning test, consistent with similar changes in other scenario tests.
core/changelog.md (1)
4-4: LGTM! Changelog accurately documents the bug fix.The changelog entry clearly describes the fix for embedding request handling in the
GetExtraFields()method.tests/core-providers/scenarios/multiple_tool_calls.go (1)
80-80: LGTM! Fallbacks properly propagated to both APIs.The addition of Fallbacks to both ChatCompletionRequest (Line 80) and ResponsesRequest (Line 93) ensures consistent fallback behavior across the dual API test framework.
tests/core-providers/scenarios/automatic_function_calling.go (1)
82-82: LGTM! Consistent fallback propagation across dual API paths.Both the ChatCompletionRequest (Line 82) and ResponsesRequest (Line 104) now include Fallbacks, ensuring consistent behavior for the automatic function calling test.
plugins/semanticcache/plugin_responses_test.go (1)
41-45: LGTM! Improved logging handles multiple content formats.The conditional logging (lines 41-45 and 64-68) gracefully handles both
ContentStrandContentBlocksformats, making the test output more robust.tests/core-providers/bedrock_test.go (2)
26-30: LGTM! Model configuration looks correct.The updated model names follow AWS Bedrock conventions, and the addition of ReasoningModel aligns with the new Reasoning scenario support.
47-49: Remove the review comment—model is confirmed available in AWS Bedrock.Claude 3.7 Sonnet is available in Amazon Bedrock in the US East (N. Virginia), US East (Ohio), and US West (Oregon) regions, with additional availability in Europe (London), Europe (Frankfurt), Europe (Ireland), Europe (Paris), Europe (Stockholm), and AWS GovCloud. The model ID in your deployments mapping ("us.anthropic.claude-3-7-sonnet-20250219-v1:0") is a valid Inference Profile that automatically routes requests to the optimal region. The fallback configuration is correct and your concern about test flakiness due to unavailability is unfounded.
tests/core-providers/config/account.go (2)
128-141: LGTM! Bedrock configuration properly structured.The deployments mapping includes both the new claude-sonnet-4 and claude-3.7-sonnet models, and the models list correctly specifies the concrete model identifiers that don't require deployment mapping.
156-183: Verify that Azure tests properly skip when new environment variables are not set.The original review comment raises valid concerns about a breaking change, but I was unable to locate test files to verify the claims about proper environment variable gating. The original comment requests verification of:
- Whether tests properly skip when
AZURE_EMB_API_KEYandAZURE_EMB_ENDPOINTare not set- Whether this breaking change is documented in the PR description and migration guide
Please manually verify:
- Do test files have skip conditions checking for the new
AZURE_EMB_*environment variables?- Is the breaking change clearly documented for users?
- Will existing tests pass or fail if the new credentials are not provided?
docs/features/plugins/mocker.mdx (1)
42-70: Excellent documentation! Clear examples and comprehensive coverage.The documentation effectively demonstrates:
- ResponsesRequest support alongside ChatCompletionRequest
- Universal mock rules that work for both request types
- skip-mocker context key for per-request bypassing
- Proper usage examples with correct context key type
The troubleshooting sections are particularly helpful for addressing common issues.
Also applies to: 105-127, 180-201
plugins/semanticcache/test_utils.go (3)
114-298: Well-structured mock rules with comprehensive coverage.The mock rules cover a wide range of test scenarios:
- Core prompts (ML, AI, Bifrost definitions)
- Weather, blockchain, quantum computing
- Conversational patterns
- Test-specific prompts with flexible regex patterns
- Default catch-all rule
The regex patterns are appropriately case-insensitive and the probability is consistently set to 1.0 for deterministic test behavior.
300-323: Mocker integration is properly implemented.The getMockedBifrostClient correctly:
- Initializes the mocker plugin with the comprehensive rules
- Creates a Bifrost client with both semantic cache and mocker plugins in the correct order
- Uses the BaseAccount for provider configuration
The plugin order (semantic cache, then mocker) ensures the cache can intercept requests before mocking occurs.
356-356: Good clarification on hybrid approach.The comment clearly explains that Weaviate is retained for real embeddings while the mocker handles chat completions. This is the correct approach since the semantic cache needs real vector similarity computations for embeddings.
plugins/mocker/main.go (4)
495-502: LGTM! Skip-mocker context key properly implemented.The skip-mocker feature is correctly implemented:
- Uses the proper
schemas.BifrostContextKeytype for the context key- Checks the boolean value correctly
- Returns early before any processing
- Restricts processing to ChatCompletionRequest and ResponsesRequest types only
This aligns with the documentation in
mocker.mdx.
613-617: LGTM! Loop variable renamed to avoid shadowing.Renaming the loop variable from
modeltoconditionModelprevents shadowing the outermodelvariable fromreq.GetRequestFields(). This improves code clarity and prevents potential bugs.
806-855: ResponsesRequest support properly implemented with correct schema mapping.The code correctly branches on request type and generates appropriate response structures:
- ChatCompletionRequest → BifrostChatResponse
- ResponsesRequest → BifrostResponsesResponse
The ResponsesResponse includes:
- Proper message structure with role and content
- Usage mapping (InputTokens, OutputTokens, TotalTokens)
- ExtraFields with correct RequestType
Good use of
bifrost.Ptrfor pointer fields and proper timestamp generation.
863-875: GetExtraFields() already handles ResponsesResponse correctly—review comment is outdated.The
GetExtraFields()implementation incore/schemas/bifrost.go(lines 225–248) includes an explicit case forResponsesResponseat lines 231–232, returning&r.ResponsesResponse.ExtraFieldsindependently. The code inplugins/mocker/main.gocorrectly relies on this method without requiring explicit handling for both response types.
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
0b28240 to
e2c35c4
Compare
Pratham-Mishra04
force-pushed
the
10-22-docs_documentation_fixes_and_updates
branch
from
c0b9a3d to
ad29aa4
Compare
Pratham-Mishra04
changed the base branch from
10-22-docs_documentation_fixes_and_updates
to
graphite-base/661
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
e2c35c4 to
b966678
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/661
branch
from
ad29aa4 to
427f5c2
Compare
Pratham-Mishra04
changed the base branch from
graphite-base/661
to
10-22-feat_enhacements_token_details_for_chat_completions_and_added_latency_calculation_in_vertex
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
b966678 to
beba46d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/core-providers/scenarios/validation_presets.go (1)
425-469: CombineExpectations cannot turn off booleans (including ShouldHaveLatency).Pattern uses “copy if true”, so later presets can’t set a flag to false (e.g., ShouldHaveContent, ShouldHaveLatency). This breaks intended “later overrides earlier”. Recommend tri-state or explicit override semantics.
Minimal fix for latency (apply similarly to other bools if needed):
- if exp.ShouldHaveLatency { - base.ShouldHaveLatency = exp.ShouldHaveLatency - } + // Always override latency expectation explicitly + base.ShouldHaveLatency = exp.ShouldHaveLatencydocs/features/plugins/mocker.mdx (1)
42-71: Minimal example: fix Init error var and define pointer helpers.
- The code assigns initErr but checks err. Use initErr in the error check.
- Examples use stringPtr later; define it or use bifrost.Ptr consistently.
Suggested tweaks:
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, }) - if err != nil { - panic(err) - } + if initErr != nil { + panic(initErr) + } + // helper if you prefer not to use bifrost.Ptr + stringPtr := func(s string) *string { return &s }plugins/mocker/main.go (3)
521-524: Latency sleep applied but not recorded in responses.Validators now require latency. Record the simulated latency (ms) into ExtraFields.Latency on the mock response.
Minimal change (capture delay and pass it through):
- if latency := p.getLatency(&rule.MockRule); latency != nil { - delay := p.calculateLatency(latency) - time.Sleep(delay) - } + var delay time.Duration + if latency := p.getLatency(&rule.MockRule); latency != nil { + delay = p.calculateLatency(latency) + time.Sleep(delay) + } ... - return p.generateSuccessShortCircuit(req, response) + return p.generateSuccessShortCircuit(req, response, int64(max(1, delay/time.Millisecond)))And update generator to set ExtraFields.Latency (see next comment).
806-855: NPE + missing latency in success response; handle per request type.
- NPE risk: overriding model unconditionally uses ChatResponse; for ResponsesRequest, ChatResponse is nil.
- Set ExtraFields.Latency for both branches.
Suggested refactor:
- func (p *MockerPlugin) generateSuccessShortCircuit(req *schemas.BifrostRequest, response *Response) ... + func (p *MockerPlugin) generateSuccessShortCircuit(req *schemas.BifrostRequest, response *Response, latencyMs int64) ... @@ - mockResponse := &schemas.BifrostResponse{} + mockResponse := &schemas.BifrostResponse{} @@ if req.RequestType == schemas.ChatCompletionRequest { mockResponse.ChatResponse = &schemas.BifrostChatResponse{ Model: model, Usage: &usage, Choices: []schemas.BifrostResponseChoice{ ... }, ExtraFields: schemas.BifrostResponseExtraFields{ RequestType: schemas.ChatCompletionRequest, Provider: provider, ModelRequested: model, + Latency: latencyMs, }, } } else if req.RequestType == schemas.ResponsesRequest { mockResponse.ResponsesResponse = &schemas.BifrostResponsesResponse{ CreatedAt: int(time.Now().Unix()), Output: []schemas.ResponsesMessage{ ... }, Usage: &schemas.ResponsesResponseUsage{ ... }, ExtraFields: schemas.BifrostResponseExtraFields{ RequestType: schemas.ResponsesRequest, Provider: provider, ModelRequested: model, + Latency: latencyMs, }, } } @@ - if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + if content.Model != nil { + switch req.RequestType { + case schemas.ChatCompletionRequest: + if mockResponse.ChatResponse != nil { mockResponse.ChatResponse.Model = *content.Model } + case schemas.ResponsesRequest: + if mockResponse.ResponsesResponse != nil { mockResponse.ResponsesResponse.ExtraFields.ModelRequested = *content.Model } + } + }Also applies to: 858-861
983-1029: DefaultBehaviorSuccess always returns ChatResponse (wrong for ResponsesRequest).Return a ResponsesResponse when req.RequestType == ResponsesRequest.
Patch:
- case DefaultBehaviorSuccess: + case DefaultBehaviorSuccess: finishReason := "stop" - return req, &schemas.PluginShortCircuit{ - Response: &schemas.BifrostResponse{ - ChatResponse: &schemas.BifrostChatResponse{ ... }, - }, - }, nil + if req.RequestType == schemas.ResponsesRequest { + return req, &schemas.PluginShortCircuit{ + Response: &schemas.BifrostResponse{ + ResponsesResponse: &schemas.BifrostResponsesResponse{ + CreatedAt: int(time.Now().Unix()), + Output: []schemas.ResponsesMessage{ + { Role: bifrost.Ptr(schemas.ResponsesInputMessageRoleAssistant), + Content: &schemas.ResponsesMessageContent{ ContentStr: bifrost.Ptr("Mock plugin default response") }, + Type: bifrost.Ptr(schemas.ResponsesMessageTypeMessage), + }, + }, + ExtraFields: schemas.BifrostResponseExtraFields{ + RequestType: schemas.ResponsesRequest, Provider: provider, ModelRequested: model, + }, + }, + }, + }, nil + } + return req, &schemas.PluginShortCircuit{ + Response: &schemas.BifrostResponse{ + ChatResponse: &schemas.BifrostChatResponse{ ... as before ... }, + }, + }, nil
♻️ Duplicate comments (2)
tests/core-providers/cohere_test.go (1)
41-41: Update the misleading comment.The boolean value was changed from
truetofalse, but the comment still referencesc4ai-aya-vision-8b modelwhile the test usescommand-a-vision-07-2025(line 27). This issue was previously flagged for lines 39-41.Please update the comment to either:
- Reference the actual model under test (
command-a-vision-07-2025)- Clarify what
falsemeans (e.g., "Not supported by command-a-vision-07-2025")- Remove the comment if it no longer adds value
plugins/governance/version (1)
1-1: Governance version is ahead of peer plugins.The existing review comment correctly identifies that governance is being bumped to 1.3.16 while peer plugins in the 1.3.x series (telemetry, semanticcache, logging, jsonparser, mocker) are only at 1.3.15. Please confirm whether this version skew is intentional or if governance should be aligned at 1.3.15 with the others.
🧹 Nitpick comments (14)
tests/core-providers/cross_provider_test.go (1)
12-14: Remove the redundantreturnstatement and clarify the skip intent.Line 14's
returnis unnecessary—t.Skip()already terminates test execution. Additionally, the unconditional skip with no environment guard or comment leaves the extensive test setup code below (lines 16–113) as dead code. If this skip is temporary or conditional, consider adding an environment variable check; if it's permanent, remove the dead code to reduce maintenance burden.Apply this minimal fix to remove the redundancy:
func TestCrossProviderScenarios(t *testing.T) { t.Skip("Skipping cross provider scenarios test") - returnIf the skip should be conditional based on an environment flag (as suggested by the PR's mention of environment-based test guards), consider:
func TestCrossProviderScenarios(t *testing.T) { if os.Getenv("RUN_CROSS_PROVIDER_TESTS") == "" { t.Skip("Skipping cross provider scenarios test (set RUN_CROSS_PROVIDER_TESTS to enable)") } // ... test body }tests/core-providers/vertex_test.go (1)
13-15: LGTM! Test guard logic is correct.The skip condition properly handles Vertex AI's multiple authentication methods (API key or project credentials). The test will run when either
VERTEX_API_KEYis set or bothVERTEX_PROJECT_IDandVERTEX_CREDENTIALSare provided.Optionally, the skip message could be more concise:
- t.Skip("Skipping Vertex tests because VERTEX_API_KEY is not set and VERTEX_PROJECT_ID or VERTEX_CREDENTIALS is not set") + t.Skip("Skipping Vertex tests: set VERTEX_API_KEY or both VERTEX_PROJECT_ID and VERTEX_CREDENTIALS")tests/core-providers/gemini_test.go (1)
13-14: Optional: Consider extracting the environment guard pattern into a helper function.The environment check pattern is duplicated across multiple provider test files. While the current approach is clear and explicit, you could optionally extract this into a helper function in the
configpackage to reduce duplication.For example:
// In tests/core-providers/config/setup.go func SkipIfEnvNotSet(t *testing.T, envVar, providerName string) { if os.Getenv(envVar) == "" { t.Skipf("Skipping %s tests because %s is not set", providerName, envVar) } }Then use it as:
func TestGemini(t *testing.T) { config.SkipIfEnvNotSet(t, "GEMINI_API_KEY", "Gemini") // ... }This is a minor refactor that could be deferred or omitted if you prefer the current explicit approach.
tests/core-providers/scenarios/speech_synthesis_stream.go (1)
182-189: Mirror the final-latency assertion in advanced stream tests.Replicate this last-chunk latency check in LongText_HDModel_Streaming and MultipleVoices_Streaming to keep parity across scenarios.
tests/core-providers/scenarios/simple_chat.go (1)
104-104: Add Fallbacks to the Chat Completions path for parity.Responses has Fallbacks; Chat path doesn’t here (other scenarios include it). Add for consistency and behavior symmetry.
Apply this diff near the chat request construction:
chatReq := &schemas.BifrostChatRequest{ Provider: testConfig.Provider, Model: testConfig.ChatModel, Input: chatMessages, Params: &schemas.ChatParameters{ MaxCompletionTokens: bifrost.Ptr(150), }, + Fallbacks: testConfig.Fallbacks, }tests/core-providers/scenarios/chat_completion_stream.go (1)
111-113: Remove redundant nil-check for Delta path.You already continue when ChatStreamResponseChoice is nil. Simplify the inner guard.
Apply this diff:
- if choice.ChatStreamResponseChoice != nil && choice.ChatStreamResponseChoice.Delta != nil { + if choice.ChatStreamResponseChoice.Delta != nil {tests/core-providers/scenarios/validation_presets.go (2)
164-164: Speech latency required — OK.No issues; just ensure speech code populates latency consistently.
183-183: Transcription latency required — OK.Same note as speech.
docs/features/plugins/mocker.mdx (3)
105-127: “Responses Request Example” compiles — just ensure pointer helper is available.This uses stringPtr; either include its definition (as above) or use bifrost.Ptr for strings.
180-201: Supported request types section is clear — add one-liner about default behavior.State that default behavior “success” returns a Chat response unless otherwise noted; or update plugin to emit ResponsesResponse for ResponsesRequest (see code review).
535-546: Skip-mocker context key guidance is great — add exported const to prevent typos.Consider documenting/adding a constant key in schemas (e.g., schemas.ContextKeySkipMocker).
tests/core-providers/scenarios/response_validation.go (2)
493-501: Latency validation may be too strict (<=0 treated as failure).Some fast/mocked paths can legitimately measure 0ms. Recommend “present and ≥0” with a warning for 0 rather than hard-fail, or enforce >0 only when not mocked.
Example pattern:
- if response.ExtraFields.Latency <= 0 { - result.Passed = false - result.Errors = append(result.Errors, "Expected latency information but not present or invalid") - } else { + if response.ExtraFields.Latency < 0 { + result.Passed = false + result.Errors = append(result.Errors, "Latency invalid (negative)") + } else { + if response.ExtraFields.Latency == 0 { + result.Warnings = append(result.Warnings, "Latency is 0ms") + } result.MetricsCollected["latency_ms"] = response.ExtraFields.Latency }Also applies to: 666-674, 843-851, 909-917, 974-982, 1027-1035
1045-1050: Dimension probe is safe — minor clarity nit.Optional: guard Embedding2DArray nil/len in a separate if for readability.
plugins/mocker/main.go (1)
495-503: Skip-mocker and request-type gating — LGTM.Early exit and limiting to ChatCompletion/Responses reduce surprises. Consider a shared constant for the context key to avoid typos across codebases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
.github/workflows/scripts/release-core.sh(1 hunks)core/bifrost.go(1 hunks)core/changelog.md(1 hunks)core/schemas/bifrost.go(1 hunks)core/version(1 hunks)docs/architecture/core/concurrency.mdx(1 hunks)docs/features/plugins/mocker.mdx(5 hunks)framework/changelog.md(1 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/main.go(6 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(0 hunks)plugins/semanticcache/plugin_responses_test.go(3 hunks)plugins/semanticcache/test_utils.go(4 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/anthropic_test.go(2 hunks)tests/core-providers/azure_test.go(2 hunks)tests/core-providers/bedrock_test.go(2 hunks)tests/core-providers/cerebras_test.go(2 hunks)tests/core-providers/cohere_test.go(3 hunks)tests/core-providers/config/account.go(3 hunks)tests/core-providers/cross_provider_test.go(1 hunks)tests/core-providers/gemini_test.go(1 hunks)tests/core-providers/groq_test.go(2 hunks)tests/core-providers/mistral_test.go(2 hunks)tests/core-providers/ollama_test.go(2 hunks)tests/core-providers/openai_test.go(2 hunks)tests/core-providers/openrouter_test.go(1 hunks)tests/core-providers/parasail_test.go(2 hunks)tests/core-providers/scenarios/automatic_function_calling.go(2 hunks)tests/core-providers/scenarios/chat_completion_stream.go(4 hunks)tests/core-providers/scenarios/cross_provider_scenarios.go(1 hunks)tests/core-providers/scenarios/embedding.go(1 hunks)tests/core-providers/scenarios/multiple_tool_calls.go(2 hunks)tests/core-providers/scenarios/reasoning.go(1 hunks)tests/core-providers/scenarios/response_validation.go(9 hunks)tests/core-providers/scenarios/responses_stream.go(3 hunks)tests/core-providers/scenarios/simple_chat.go(1 hunks)tests/core-providers/scenarios/speech_synthesis_stream.go(2 hunks)tests/core-providers/scenarios/text_completion_stream.go(2 hunks)tests/core-providers/scenarios/transcription_stream.go(2 hunks)tests/core-providers/scenarios/validation_presets.go(5 hunks)tests/core-providers/sgl_test.go(2 hunks)tests/core-providers/vertex_test.go(2 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_edge_cases_test.go
🧰 Additional context used
🧬 Code graph analysis (10)
tests/core-providers/scenarios/responses_stream.go (2)
core/schemas/responses.go (1)
BifrostResponsesStreamResponse(1330-1363)plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/scenarios/speech_synthesis_stream.go (2)
core/schemas/speech.go (1)
BifrostSpeechStreamResponse(107-112)plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/scenarios/transcription_stream.go (1)
core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(89-96)
plugins/mocker/main.go (3)
core/schemas/bifrost.go (6)
BifrostContextKey(96-96)RequestType(79-79)ChatCompletionRequest(84-84)ResponsesRequest(86-86)BifrostResponse(213-223)BifrostResponseExtraFields(251-259)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)ResponsesInputMessageRoleAssistant(304-304)ResponsesMessageTypeMessage(263-263)
tests/core-providers/scenarios/response_validation.go (1)
plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/scenarios/chat_completion_stream.go (2)
core/schemas/chatcompletions.go (2)
BifrostChatResponse(20-30)ChatStreamResponseChoice(518-520)tests/core-providers/scenarios/utils.go (1)
CreateBasicChatMessage(218-225)
tests/core-providers/scenarios/text_completion_stream.go (2)
core/schemas/textcompletions.go (1)
BifrostTextCompletionResponse(64-72)plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/config/account.go (3)
core/schemas/account.go (1)
AzureKeyConfig(21-25)ui/lib/types/config.ts (1)
AzureKeyConfig(20-24)core/utils.go (1)
Ptr(12-14)
plugins/semanticcache/test_utils.go (4)
plugins/mocker/main.go (7)
MockRule(77-85)Conditions(89-94)Response(98-104)ResponseTypeSuccess(28-28)SuccessResponse(108-115)MockerConfig(68-73)Init(156-202)plugins/semanticcache/main.go (2)
Plugin(137-144)Init(263-327)core/bifrost.go (2)
Bifrost(34-51)Init(80-194)core/schemas/bifrost.go (1)
BifrostConfig(21-29)
tests/core-providers/bedrock_test.go (1)
core/schemas/bifrost.go (2)
Fallback(114-117)Bedrock(38-38)
🪛 LanguageTool
plugins/otel/changelog.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...his brings down inference overhead to < 1µseconds
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (9)
- 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 (73)
tests/core-providers/scenarios/cross_provider_scenarios.go (1)
700-701: LGTM!The whitespace/indentation adjustment improves comment alignment. No functional changes.
tests/core-providers/mistral_test.go (1)
13-15: LGTM! Defensive test guard correctly implemented.The environment check correctly skips the test when
MISTRAL_API_KEYis unavailable, avoiding unnecessary setup and providing a clear skip message. The placement beforeSetupTest()is optimal.tests/core-providers/groq_test.go (2)
4-4: LGTM!The
osimport is necessary for the environment variable check added below.
13-15: LGTM!The environment-based guard is a best practice for integration tests requiring API credentials. The condition correctly checks for an unset or empty environment variable, and the skip message is clear and informative. Placing the guard before test setup ensures efficient resource usage.
tests/core-providers/openai_test.go (2)
4-4: LGTM!The
osimport is necessary for the environment variable check and is correctly placed.
13-16: LGTM! Good practice for optional integration tests.The environment guard correctly skips the test when
OPENAI_API_KEYis not available, preventing failures in environments without credentials. The implementation is idiomatic and the skip message is clear.tests/core-providers/ollama_test.go (2)
4-4: LGTM!The
osimport is necessary for the environment variable check added below.
13-15: LGTM! Good practice for integration tests.The environment guard properly skips the test when the required configuration is unavailable, making the test suite more flexible and preventing false failures.
tests/core-providers/parasail_test.go (2)
4-4: LGTM: Import added for environment check.The
osimport is correctly added to support the environment variable check below.
13-16: LGTM: Well-implemented environment-based test guard.This check prevents test failures when the API key is unavailable and aligns with the consistent pattern applied across other provider tests in this PR. The guard is efficiently placed before expensive setup operations and provides a clear skip message.
tests/core-providers/sgl_test.go (2)
4-4: LGTM!The
osimport is necessary for the environment variable check and is correctly placed in the import block.
13-15: LGTM! Good test hygiene.The environment guard prevents test failures in unconfigured environments and avoids unnecessary setup. This aligns with the broader pattern in this PR of making tests environment-aware and more robust in CI/CD pipelines.
tests/core-providers/vertex_test.go (1)
4-4: LGTM!The
osimport is correctly added to support the environment variable checks.tests/core-providers/anthropic_test.go (1)
4-4: LGTM! Environment-based test guard added.The addition of the environment check prevents test failures when the API key is unavailable and aligns with the pattern used across other provider tests.
Also applies to: 13-15
tests/core-providers/azure_test.go (1)
4-4: LGTM! Environment-based test guard added.The environment check follows the established pattern and prevents test failures when credentials are missing.
Also applies to: 13-15
tests/core-providers/openrouter_test.go (1)
14-14: LGTM! Skip message standardized.The updated message format is clearer and aligns with the standardized format used across other provider tests in this PR.
tests/core-providers/cerebras_test.go (1)
4-4: LGTM! Environment-based test guard added.The guard implementation is correct and follows the established pattern across provider tests.
Also applies to: 13-15
tests/core-providers/gemini_test.go (1)
14-14: LGTM! Skip message standardized.The message format is now consistent with other provider tests in this PR.
tests/core-providers/cohere_test.go (1)
4-4: LGTM! Environment-based test guard added.The addition of the
osimport and the environment variable check to skip tests whenCOHERE_API_KEYis not set follows the pattern established across other provider tests in this PR. This is good practice for conditional test execution.Also applies to: 13-16
framework/changelog.md (1)
4-4: LGTM!The changelog entry correctly reflects the coordinated version bump from 1.2.12 to 1.2.13.
core/version (1)
1-1: LGTM!The version bump to 1.2.13 aligns with the coordinated release noted in the PR objectives.
plugins/jsonparser/version (1)
1-1: LGTM!The version bump to 1.3.15 is consistent with other plugin updates in the 1.3.x series.
plugins/maxim/version (1)
1-1: LGTM!The version bump to 1.4.15 is consistent with the coordinated release.
plugins/telemetry/version (1)
1-1: LGTM!The version bump to 1.3.15 aligns with other plugins in the 1.3.x series.
plugins/maxim/changelog.md (1)
4-4: LGTM!The changelog entry correctly reflects the updated core and framework versions (1.2.13 and 1.1.15 respectively).
transports/version (1)
1-1: LGTM!The version bump to 1.3.10 is consistent with the coordinated release.
plugins/jsonparser/changelog.md (1)
4-4: LGTM!The changelog entry correctly documents the coordinated version bump across core and framework.
plugins/logging/version (1)
1-1: LGTM!Version bump aligns with the coordinated patch release across the repository.
plugins/otel/version (1)
1-1: LGTM!Version increment is consistent with the coordinated release.
plugins/governance/changelog.md (1)
4-4: LGTM!Changelog entry properly documents the version update.
framework/version (1)
1-1: LGTM!Framework version bump is part of the coordinated patch release.
core/schemas/bifrost.go (1)
235-236: LGTM! Bug fix correctly implemented.The addition of the
EmbeddingResponsecase completes the switch statement coverage and fixes the issue where embedding requests would previously fall through to the default case, returning an emptyBifrostResponseExtraFields{}. The implementation follows the established pattern used by all other response types.plugins/logging/changelog.md (1)
4-4: LGTM!Changelog correctly documents the version bump.
plugins/telemetry/changelog.md (1)
4-4: LGTM!Changelog entry properly reflects the coordinated version update.
core/changelog.md (1)
4-6: LGTM!The changelog entries are clear, concise, and properly formatted. They accurately document the bug fixes and features in this release.
plugins/otel/changelog.md (1)
4-6: LGTM!The changelog entries clearly document the version bump and new features. The static analysis warning about "µseconds" is a false positive—the Greek letter mu (µ) is the correct symbol for "micro" in microseconds.
core/bifrost.go (1)
1232-1236: LGTM!The improved error logging correctly prioritizes the structured error message when available, falling back to the generic format otherwise. This defensive pattern prevents nil pointer dereferences and provides clearer diagnostics.
plugins/semanticcache/plugin_responses_test.go (2)
41-45: LGTM!The defensive logging properly handles both ContentStr and ContentBlocks response formats, preventing potential nil pointer dereferences while providing useful test output.
64-68: LGTM!Consistent application of the defensive logging pattern from the first response handling.
transports/changelog.md (1)
4-9: LGTM!The changelog entries comprehensively document the transport layer changes, including version bumps, new features, and bug fixes. All entries are clear and follow the established format.
tests/core-providers/config/account.go (2)
128-130: LGTM!The Bedrock deployment mappings are updated to include the latest model versions, aligning with the test configuration changes.
156-183: LGTM!The Azure configuration restructuring improves separation of concerns by splitting chat and embedding endpoints. The APIVersion field with environment variable override (AZURE_API_VERSION) provides good flexibility for testing different API versions.
tests/core-providers/bedrock_test.go (2)
26-30: LGTM!The model updates align with the Bedrock configuration changes in config/account.go. The full ARN-style model identifiers are correct for Bedrock, and adding the ReasoningModel enables testing of reasoning capabilities.
45-49: LGTM!Enabling the Reasoning scenario is consistent with the addition of the ReasoningModel. The fallback configuration to claude-3.7-sonnet provides resilience for testing.
.github/workflows/scripts/release-core.sh (1)
38-42: Environment variable gating and error handling are correctly implemented.Verification confirms:
- All test files skip gracefully when credentials are missing (pattern:
if os.Getenv("OPENAI_API_KEY") == "" { t.Skip(...) })- Release script uses
set -euo pipefail, ensuring test failures block releases while skipped tests pass through- The
-vflag provides beneficial visibility in CI logsplugins/semanticcache/test_utils.go (5)
14-14: LGTM! Clean import addition.The mocker plugin import is properly added to support the new mock-based testing infrastructure.
114-298: Well-structured mock rules with comprehensive coverage.The mock rules cover a wide range of test scenarios effectively. A few observations:
Line 148: The exact match
"What is the capital of France\\?"is very specific and won't match variations like "What's the capital of France?" Consider using a more flexible regex if variations are expected.Line 252: The regex pattern
"test.*message|test.*no-store|test.*cache|test.*error|ttl test|threshold test|provider.*test|edge case test"is quite broad and could match unintended strings. Consider if this is intentional or if more specific patterns would be better.Lines 287-296: The catch-all default rule correctly uses
Priority: -1to ensure it's matched last, which is good defensive design.Overall, the implementation looks solid for the test scenarios described in the PR.
300-323: Excellent mocker integration for test isolation.The function properly initializes a Bifrost client with mocker support. The plugin ordering (semantic cache before mocker) is correct, allowing the semantic cache to intercept requests in PreHook before the mocker provides responses in PostHook.
This approach effectively decouples tests from external API dependencies while still validating semantic cache behavior.
352-385: Clean integration of mocked client into test setup.The modification correctly separates concerns:
- Weaviate remains for actual embedding operations (line 356 comment is helpful)
- Mocked Bifrost client is used for chat completions (line 376)
This approach maintains test coverage for embedding functionality while isolating chat completion tests from external API dependencies.
698-811: Well-designed retry infrastructure with proper error handling.The retry helpers implement exponential backoff correctly and appropriately skip tests (rather than fail them) when external API calls fail after all retries. This is the right behavior for test flakiness mitigation.
Key strengths:
- Generic
WithRetries[T any]for type safety- Consistent retry configuration across all request types
- Detailed logging at each attempt
- Proper timeout handling for streaming requests
plugins/mocker/version (1)
1-1: LGTM! Version bump aligns with changelog.The version bump from 1.3.14 to 1.3.15 is consistent with the new features documented in the changelog (responses request support and skip-mocker context key).
tests/core-providers/scenarios/reasoning.go (1)
49-49: LGTM! Consistent Fallbacks field addition.The addition of the Fallbacks field to the ResponsesRequest is consistent with similar changes across other test scenarios in this PR, enabling fallback behavior in reasoning tests.
plugins/mocker/changelog.md (1)
4-6: LGTM! Changelog accurately reflects PR changes.The changelog properly documents:
- Version bumps for core and framework
- New responses request support feature
- New skip-mocker context key feature
These align with the PR objectives and related code changes.
plugins/semanticcache/changelog.md (1)
4-5: LGTM! Changelog accurately documents test infrastructure changes.The changelog properly captures:
- Coordinated version bumps across core and framework
- Integration of mocker plugin into semantic cache tests
This aligns with the substantial changes in
test_utils.go.plugins/semanticcache/version (1)
1-1: LGTM! Coordinated version bump.The version bump from 1.3.14 to 1.3.15 is consistent with the coordinated release across multiple plugins (mocker, semanticcache, etc.).
tests/core-providers/scenarios/automatic_function_calling.go (2)
82-82: LGTM! Consistent Fallbacks support for Chat API.Addition of Fallbacks field enables fallback behavior testing in the Chat Completions API path for automatic function calling scenarios.
104-104: LGTM! Consistent Fallbacks support for Responses API.Addition of Fallbacks field enables fallback behavior testing in the Responses API path, maintaining parity with the Chat Completions API path.
tests/core-providers/scenarios/text_completion_stream.go (2)
106-108: Good addition of per-chunk latency observability.Logging latency for each streaming chunk provides valuable debugging information. The comment correctly notes that intermediate chunks may have 0 latency, with the final chunk containing the total.
176-183: Proper validation of streaming latency in final chunk.The validation correctly ensures that:
- The last streaming chunk includes latency information (line 178)
- The total latency is logged when present (line 181)
This aligns with the broader PR improvements to latency tracking and is consistent with similar validations added to other streaming test scenarios.
tests/core-providers/scenarios/transcription_stream.go (1)
181-183: Latency logging and final-latency check look good.Per-chunk logs and asserting last-chunk latency > 0 are sound and safe given prior nil checks. No issues.
Also applies to: 220-227
tests/core-providers/scenarios/speech_synthesis_stream.go (1)
150-152: Per-chunk latency logging: LGTM.Guarded by payload checks; low risk.
tests/core-providers/scenarios/multiple_tool_calls.go (1)
80-81: Fallbacks propagation to both APIs: LGTM.Consistent with other scenarios and PR goals.
Also applies to: 93-94
tests/core-providers/scenarios/responses_stream.go (3)
108-110: Per-chunk latency logging: LGTM.Non-blocking and helpful for observability.
206-214: Final-latency check: LGTM.Asserting last-chunk latency > 0 is appropriate.
248-248: Prompt tweak to Celsius: LGTM.Clarifies expected units and aids tool-call determinism.
tests/core-providers/scenarios/chat_completion_stream.go (2)
97-99: Latency logging and final-latency validation: LGTM.Checks are correct and guarded; good coverage.
Also applies to: 199-207
226-226: Prompt tweak to Celsius: LGTM.Improves determinism for weather tool.
tests/core-providers/scenarios/validation_presets.go (2)
25-25: Latency expectation enabled globally for Chat — verify provider coverage.Good default. Ensure all provider paths populate ExtraFields.Latency (>0 ms) to avoid flaky tests, including mocks. I see plugin support added, but mocked responses also need latency filled.
114-114: Embeddings now require latency — align generators.Looks fine; confirm embedding paths (including any local/test stubs) set ExtraFields.Latency.
tests/core-providers/scenarios/response_validation.go (2)
41-41: New ShouldHaveLatency flag — good addition.Field placement and intent are clear.
1002-1007: Embedding data nil/len check — LGTM.Using len(response.Data) == 0 is idiomatic and safe.
docs/architecture/core/concurrency.mdx (1)
271-295: Mermaid pool flow updates — LGTM.Diagram changes improve clarity (PoolCheck path, GC interaction).
plugins/mocker/main.go (1)
598-616: No action required — Go version requirement already satisfied.The
plugins/mocker/go.modspecifiesgo 1.24.1andtoolchain go1.24.3, which exceeds the Go 1.21+ requirement forslices.Containsandmaps.Copy. The code is compliant.
Pratham-Mishra04
force-pushed
the
10-22-feat_enhacements_token_details_for_chat_completions_and_added_latency_calculation_in_vertex
branch
from
427f5c2 to
fbdd429
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
beba46d to
414f985
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
tests/core-providers/scenarios/speech_synthesis.go (2)
59-117: Fix loop variable capture before callingt.Parallel.
Whent.Parallel()runs, the closure captures the loop variabletc, which is reused on every iteration. All subtests end up seeing the final element, so most cases never execute with their intended data. Rebindtcinside the loop before invokingt.Run.- for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() }
210-249: RebindvoiceTypebefore parallel subtests.
Same closure-capture issue here: without rebinding, every subtest sees the lastvoiceType. AddvoiceType := voiceType(and optionally rebindvoiceif derived) beforet.Run.- for _, voiceType := range voiceTypes { - t.Run("VoiceType_"+voiceType, func(t *testing.T) { + for _, voiceType := range voiceTypes { + voiceType := voiceType + t.Run("VoiceType_"+voiceType, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() }tests/core-providers/scenarios/test_retry_framework.go (1)
234-309: Enforce fail-fast for deprecated WithTestRetry
WithTestRetry hardcodes validationResult.Passed=true and masks failures. Replace the deprecation warning in test_retry_framework.go:- t.Logf("⚠️ Warning: Using deprecated ValidateResponse function") - // skip validation - validationResult := ValidationResult{Passed: true} + t.Fatalf("WithTestRetry is deprecated; switch to specific wrappers or real validators.")Then remove or refactor its usages in tests/core-providers/scenarios/text_completion.go:57 and speech_synthesis.go:178.
tests/core-providers/scenarios/transcription.go (1)
159-166: Capture loop var and check TTS error before transcription.
- Capture
tcbeforet.Run.- Don’t ignore
GenerateTTSAudioForTesterror; fail fast if audio generation fails.- for _, tc := range customCases { - t.Run(tc.name, func(t *testing.T) { + for _, tc := range customCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() } - // Use the utility function to generate audio - audioData, _ := GenerateTTSAudioForTest(ctx, t, client, testConfig.Provider, testConfig.SpeechSynthesisModel, tc.text, "primary", "mp3") + // Use the utility function to generate audio + audioData, genErr := GenerateTTSAudioForTest(ctx, t, client, testConfig.Provider, testConfig.SpeechSynthesisModel, tc.text, "primary", "mp3") + require.NoError(t, genErr, "Failed to generate TTS audio for custom case %s", tc.name) + require.NotEmpty(t, audioData, "Generated audio is empty for %s", tc.name)docs/features/plugins/mocker.mdx (1)
25-33: Constructor name mismatch (docs vs code) and variable typo.Code exposes mocker.Init(...), but docs use NewMockerPlugin(...). Also you assign initErr but check err. Fix both to compile and match the code.
- plugin, err := mocker.NewMockerPlugin(mocker.MockerConfig{ + plugin, err := mocker.Init(mocker.MockerConfig{ Enabled: true, // Default rule will be created automatically }) if err != nil { panic(err) } @@ - client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, }) - if err != nil { + if initErr != nil { - panic(err) + panic(initErr) }Alternatively, export a NewMockerPlugin wrapper calling Init to keep the original docs intact.
Also applies to: 37-43
plugins/mocker/main.go (3)
514-518: Probability=0 rules still activate.Current logic only checks “> 0” then random draw; rules with Probability=0.0 (intended disabled) still fire. Enforce 0=disabled and use an intuitive comparison.
- // Check if rule should activate based on probability (0.0 = never, 1.0 = always) - if rule.Probability > 0 && rand.Float64() > rule.Probability { - // Rule didn't activate due to probability, continue with normal flow - return req, nil, nil - } + // Respect probability: 0.0 = never, 1.0 = always + if rule.Probability <= 0 { + return req, nil, nil + } + if rand.Float64() >= rule.Probability { + return req, nil, nil + }
857-861: Nil deref when overriding model on ResponsesRequest.You set mockResponse.ChatResponse.Model without checking the request type; for ResponsesRequest ChatResponse is nil → panic. Set model only for Chat; for Responses, update ExtraFields.ModelRequested.
- // Override model if specified - if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + // Override model if specified + if content.Model != nil { + switch req.RequestType { + case schemas.ChatCompletionRequest: + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } + case schemas.ResponsesRequest: + if mockResponse.ResponsesResponse != nil { + mockResponse.ResponsesResponse.ExtraFields.ModelRequested = *content.Model + } + } + }
982-1030: DefaultBehaviorSuccess returns ChatResponse even for ResponsesRequest.If DefaultBehavior is “success” and the request is ResponsesRequest, you return a chat payload. Return the correct shape per request type.
- case DefaultBehaviorSuccess: - finishReason := "stop" - return req, &schemas.PluginShortCircuit{ - Response: &schemas.BifrostResponse{ - ChatResponse: &schemas.BifrostChatResponse{ + case DefaultBehaviorSuccess: + finishReason := "stop" + if req.RequestType == schemas.ResponsesRequest { + return req, &schemas.PluginShortCircuit{ + Response: &schemas.BifrostResponse{ + ResponsesResponse: &schemas.BifrostResponsesResponse{ + CreatedAt: int(time.Now().Unix()), + Output: []schemas.ResponsesMessage{ + { + Role: bifrost.Ptr(schemas.ResponsesInputMessageRoleAssistant), + Type: bifrost.Ptr(schemas.ResponsesMessageTypeMessage), + Content: &schemas.ResponsesMessageContent{ + ContentStr: bifrost.Ptr("Mock plugin default response"), + }, + }, + }, + Usage: &schemas.ResponsesResponseUsage{InputTokens: 5, OutputTokens: 10, TotalTokens: 15}, + ExtraFields: schemas.BifrostResponseExtraFields{ + RequestType: schemas.ResponsesRequest, Provider: provider, ModelRequested: model, + }, + }, + }, + }, nil + } + return req, &schemas.PluginShortCircuit{ + Response: &schemas.BifrostResponse{ + ChatResponse: &schemas.BifrostChatResponse{ Model: model, Usage: &schemas.BifrostLLMUsage{ PromptTokens: 5, CompletionTokens: 10, TotalTokens: 15, }, Choices: []schemas.BifrostResponseChoice{{Index: 0, ChatNonStreamResponseChoice: &schemas.ChatNonStreamResponseChoice{ Message: &schemas.ChatMessage{ Role: schemas.ChatMessageRoleAssistant, Content: &schemas.ChatMessageContent{ContentStr: bifrost.Ptr("Mock plugin default response")}, }, }, FinishReason: &finishReason, }}, ExtraFields: schemas.BifrostResponseExtraFields{ RequestType: schemas.ChatCompletionRequest, Provider: provider, ModelRequested: model, }, }, }, }, nil
♻️ Duplicate comments (2)
tests/core-providers/scenarios/embedding.go (1)
104-112: Compile-time bug and 2D indexing hazard in embedding extraction.
for i := range responseDataLengthdoesn’t compile (cannot range over int).- When embeddings come as 2D (
Embedding2DArray), indexingresponse.Data[i]can go out-of-bounds.Apply this diff to fix both issues:
- responseDataLength := len(response.Data) + responseDataLength := len(response.Data) if responseDataLength != len(testTexts) { if responseDataLength > 0 && response.Data[0].Embedding.Embedding2DArray != nil { responseDataLength = len(response.Data[0].Embedding.Embedding2DArray) } if responseDataLength != len(testTexts) { t.Fatalf("Expected %d embedding results, got %d", len(testTexts), responseDataLength) } } - - for i := range responseDataLength { - vec, extractErr := getEmbeddingVector(response.Data[i]) + for i := 0; i < responseDataLength; i++ { + var ( + vec []float32 + extractErr error + ) + // 1D: one vector per Data item + if len(response.Data) == responseDataLength { + vec, extractErr = getEmbeddingVector(response.Data[i]) + } else { + // 2D: vectors live in Data[0].Embedding.Embedding2DArray + vec, extractErr = getEmbeddingVectorFrom2D(response.Data[0], i) + } if extractErr != nil { t.Fatalf("Failed to extract embedding vector for text '%s': %v", testTexts[i], extractErr) } if len(vec) == 0 { t.Fatalf("Embedding vector is empty for text '%s'", testTexts[i]) } embeddings[i] = vec }Add this helper in the same file (adjust types if
Embedding2DArrayis a pointer):// getEmbeddingVectorFrom2D extracts the i-th vector from a 2D embeddings payload. func getEmbeddingVectorFrom2D(d schemas.EmbeddingData, i int) ([]float32, error) { rows := d.Embedding.Embedding2DArray // [][]float32 (or *[][]float32 if pointer) if rows == nil { return nil, fmt.Errorf("2D embeddings not present") } // If pointer type, use: rows = *d.Embedding.Embedding2DArray if i < 0 || i >= len(rows) { // If pointer: len(rows) return nil, fmt.Errorf("index out of range") } return rows[i], nil // If pointer: return rows[i], nil }Based on learnings.
Also applies to: 114-123
tests/core-providers/config/account.go (1)
366-366: Ollama BaseURL: add default or gate tests.This repeats a previously raised concern: empty BaseURL if OLLAMA_BASE_URL is unset will break requests. Use a sensible default or skip Ollama tests when unset.
- BaseURL: os.Getenv("OLLAMA_BASE_URL"), + BaseURL: getEnvWithDefault("OLLAMA_BASE_URL", "http://localhost:11434"),Alternatively, ensure Ollama tests call t.Skip when OLLAMA_BASE_URL is empty.
🧹 Nitpick comments (26)
tests/core-providers/scenarios/image_base64.go (1)
23-25: Handle alternate truthy values for SKIP_PARALLEL_TESTS.At the moment only the exact literal
"true"disables parallelization. That can surprise anyone exportingTRUE/True(or similar) from their shell defaults. A case-insensitive check keeps the intent while remaining permissive.- if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + if !strings.EqualFold(os.Getenv("SKIP_PARALLEL_TESTS"), "true") { t.Parallel() }tests/core-providers/custom_test.go (1)
44-47: Prefert.Cleanupfor client shutdown.Registering the shutdown with
t.Cleanupkeeps the resource lifecycle resilient to future early returns or additional subtests; we no longer need to remember to callclient.Shutdown()manually at the end.Apply this diff:
- t.Run("CustomProviderTests", func(t *testing.T) { - runAllComprehensiveTests(t, client, ctx, testConfig) - }) - client.Shutdown() + t.Cleanup(func() { + client.Shutdown() + }) + + t.Run("CustomProviderTests", func(t *testing.T) { + runAllComprehensiveTests(t, client, ctx, testConfig) + })tests/core-providers/scenarios/tool_calls.go (1)
78-88: Consider passing Fallbacks to ResponsesRequest for parityChat requests include Fallbacks, but ResponsesRequest does not. If fallbacks are intended for both APIs (as in MultipleToolCalls), pass testConfig.Fallbacks here too for consistency, or confirm it’s intentionally omitted for this scenario.
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
76-86: Responses side lacks Fallbacks; align or documentStep 1 and Step 2 include Fallbacks for Chat but not for Responses. For parity with MultipleToolCalls (and to avoid asymmetric retries), add Fallbacks to both, or document why Responses is excluded here.
Also applies to: 192-202
tests/core-providers/scenarios/chat_completion_stream.go (1)
102-104: Per‑chunk latency logging and carry‑over: OK; consider richer aggregationLogging chunk latency and copying the last chunk’s latency into consolidatedResponse is fine. Optionally also capture:
- first non‑zero chunk latency (TTFT) and
- total stream duration,
to aid provider comparisons.Also applies to: 183-184
tests/core-providers/scenarios/response_validation.go (1)
41-42: DRY up latency validation; ensure unit consistency.Great addition of
ShouldHaveLatencywith checks across validators. The same pattern is duplicated in 6 places; extract a small helper to reduce copy/paste and keep unit handling centralized. Also ensure the unit (ms) is consistent across providers.+// validateLatency asserts latency > 0 when expected and records it. +func validateLatency(expect bool, latency int64, result *ValidationResult) { + if !expect { + return + } + if latency <= 0 { + result.Passed = false + result.Errors = append(result.Errors, "Expected latency information but not present or invalid") + return + } + result.MetricsCollected["latency_ms"] = latency +}Then replace blocks like:
if expectations.ShouldHaveLatency { if response.ExtraFields.Latency <= 0 { result.Passed = false result.Errors = append(result.Errors, "Expected latency information but not present or invalid") } else { result.MetricsCollected["latency_ms"] = response.ExtraFields.Latency } }with:
validateLatency(expectations.ShouldHaveLatency, response.ExtraFields.Latency, result)Additionally, consider using
len(response.Data) > 0consistently for “has data” metrics for embeddings to align with your earlier guard.Also applies to: 493-501, 666-674, 843-851, 909-917, 974-982, 1027-1035, 1046-1051, 1288-1292
plugins/mocker/changelog.md (1)
4-6: Tighten terminology and casing for request types.Use “ResponsesRequest” (type name) instead of “responses request” for consistency with code, and quote the context key "skip-mocker".
-- feat: added support for responses request +- feat: added support for ResponsesRequest -- feat: added "skip-mocker" context key to skip mocker plugin per request +- feat: added context key "skip-mocker" to skip the mocker plugin per requestdocs/features/plugins/mocker.mdx (3)
77-103: Unify constructor usage in examples.This section also uses NewMockerPlugin but code provides Init. Align for consistency.
-plugin, err := mocker.NewMockerPlugin(mocker.MockerConfig{ +plugin, err := mocker.Init(mocker.MockerConfig{
109-127: Undefined helper stringPtr in example.Replace stringPtr(...) with bifrost.Ptr(...) as used elsewhere.
- Conditions: mocker.Conditions{ - MessageRegex: stringPtr("(?i).*hello.*"), - }, + Conditions: mocker.Conditions{ + MessageRegex: bifrost.Ptr("(?i).*hello.*"), + },
535-546: Great troubleshooting tip. Minor formatting nit.Add code formatting to the bad context key to reduce confusion.
-// ❌ Wrong -ctx := context.WithValue(context.Background(), "skip-mocker", true) +// ❌ Wrong +ctx := context.WithValue(context.Background(), "skip-mocker", true) // key must be schemas.BifrostContextKeyplugins/mocker/main.go (1)
1042-1071: Micro-optimization: compile faker regex once.applyTemplate recompiles the faker regex each call. Hoist to a package-level var to avoid repeated compilation.
- // applyTemplate ... +var fakerVarRe = regexp.MustCompile(`\{\{faker\.([^}]+)\}\}`) +// applyTemplate ... @@ - fakerRegex := regexp.MustCompile(`\{\{faker\.([^}]+)\}\}`) - result = fakerRegex.ReplaceAllStringFunc(result, func(match string) string { + result = fakerVarRe.ReplaceAllStringFunc(result, func(match string) string { - submatch := fakerRegex.FindStringSubmatch(match) + submatch := fakerVarRe.FindStringSubmatch(match)plugins/semanticcache/test_utils.go (3)
114-298: Rule set looks solid. Add a couple of exact-match guards.Some patterns like What is the capital of France? could be anchored for fewer false positives; optional.
- Conditions: mocker.Conditions{MessageRegex: bifrost.Ptr("What is the capital of France\\?")}, + Conditions: mocker.Conditions{MessageRegex: bifrost.Ptr("^\\s*What is the capital of France\\?\\s*$")},
401-419: Prefer typed role constant.Use schemas.ChatMessageRoleUser instead of raw "user" for type safety.
- { - Role: "user", + { + Role: schemas.ChatMessageRoleUser,
754-766: Retry wrappers are fine; simplify error branches.You return the same value on both branches when err != nil. You can return directly.
- if err != nil { - if err.Error != nil { - return &schemas.BifrostResponse{ChatResponse: response}, err - } - return &schemas.BifrostResponse{ChatResponse: response}, err - } + if err != nil { + return &schemas.BifrostResponse{ChatResponse: response}, err + }(Apply similarly to Responses/Embedding/Speech variants.)
Also applies to: 769-781, 784-796, 799-811
tests/core-providers/sgl_test.go (1)
46-49: Keep client shutdown deferred to cover all code paths.Calling
client.Shutdown()immediately aftert.Runcloses the client as soon as the subtest returns. The comprehensive suite spins up a bunch of nested subtests, and if any of them callt.Parallel()(now or in the future) they’ll keep using the shared client aftert.Runhands control back to the parent. Shutting the client down here would race those goroutines. Please keep the teardown deferred (or register it witht.Cleanup) so we only close the client once the entire test tree is finished.Apply this diff:
client, ctx, cancel, err := config.SetupTest() if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + defer client.Shutdown() testConfig := config.ComprehensiveTestConfig{ Provider: schemas.SGL, @@ - t.Run("SGLTests", func(t *testing.T) { - runAllComprehensiveTests(t, client, ctx, testConfig) - }) - client.Shutdown() + t.Run("SGLTests", func(t *testing.T) { + runAllComprehensiveTests(t, client, ctx, testConfig) + })tests/core-providers/anthropic_test.go (1)
47-50: Prefer t.Cleanup for deterministic teardown.Use t.Cleanup to ensure Shutdown runs even on subtest failure and to centralize cleanup.
Apply this diff near setup and end:
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("AnthropicTests", func(t *testing.T) { + t.Run("AnthropicTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/parasail_test.go (1)
45-48: Use t.Cleanup for teardown symmetry.Align with Go testing best practice; ensures cleanup executes on failure.
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("ParasailTests", func(t *testing.T) { + t.Run("ParasailTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/mistral_test.go (1)
46-49: Harden teardown with t.Cleanup.Prevents leaks if subtest fails early.
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("MistralTests", func(t *testing.T) { + t.Run("MistralTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/gemini_test.go (2)
42-42: Confirm intent: MultipleImages disabled.Was this flipped to false due to current Gemini limitations or flakiness? If so, record the rationale via SkipReason for future re-enable.
testConfig := config.ComprehensiveTestConfig{ Provider: schemas.Gemini, @@ Scenarios: config.TestScenarios{ @@ - MultipleImages: false, + MultipleImages: false, }, + SkipReason: "Gemini MultipleImages disabled temporarily; revisit when stable", }
52-55: Use t.Cleanup for shutdown.Centralizes teardown and runs on failure.
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("GeminiTests", func(t *testing.T) { + t.Run("GeminiTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/openrouter_test.go (2)
36-41: Document Beta gating for disabled scenarios.You’re disabling several scenarios due to Responses API Beta. Encode the rationale to aid future re‑enable.
testConfig := config.ComprehensiveTestConfig{ @@ Scenarios: config.TestScenarios{ @@ - End2EndToolCalling: false, // OpenRouter's responses API is in Beta + End2EndToolCalling: false, // OpenRouter's responses API is in Beta @@ - CompleteEnd2End: false, // OpenRouter's responses API is in Beta + CompleteEnd2End: false, // OpenRouter's responses API is in Beta }, + SkipReason: "OpenRouter responses API in Beta; image and end-to-end scenarios disabled", }
45-48: Adopt t.Cleanup for teardown.Consistent with other tests; ensures cleanup on failure.
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("OpenRouterTests", func(t *testing.T) { + t.Run("OpenRouterTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/bedrock_test.go (2)
25-30: Model identifier consistency: friendly vs Bedrock IDs.You mix friendly names (e.g., "claude-sonnet-4") and full Bedrock IDs (e.g., "anthropic.claude-3-5-sonnet-20240620-v1:0"). That’s fine if your key Deployments map resolves friendlies, but it’s easy to misconfigure.
- Confirm runAllComprehensiveTests resolves both forms under Bedrock.
- Optionally standardize on friendly names and rely on Deployments, or standardize on full IDs across Chat/Vision/Reasoning to reduce confusion.
Also applies to: 39-45, 46-48
51-54: Switch to t.Cleanup for shutdown.Keeps teardown robust if the subtest fails.
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("BedrockTests", func(t *testing.T) { + t.Run("BedrockTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/config/account.go (1)
60-66: Nice utility.getEnvWithDefault is a good addition; consider moving to a shared utils pkg if reused elsewhere.
tests/core-providers/azure_test.go (1)
46-49: Use t.Cleanup for consistent teardown.Ensures Shutdown always runs.
@@ if err != nil { t.Fatalf("Error initializing test setup: %v", err) } defer cancel() + t.Cleanup(func() { client.Shutdown() }) @@ - t.Run("AzureTests", func(t *testing.T) { + t.Run("AzureTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
.github/workflows/scripts/release-core.sh(1 hunks)core/bifrost.go(1 hunks)core/changelog.md(1 hunks)core/schemas/bifrost.go(1 hunks)core/version(1 hunks)docs/architecture/core/concurrency.mdx(1 hunks)docs/features/plugins/mocker.mdx(5 hunks)framework/changelog.md(1 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/main.go(6 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(0 hunks)plugins/semanticcache/plugin_responses_test.go(3 hunks)plugins/semanticcache/test_utils.go(4 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/anthropic_test.go(3 hunks)tests/core-providers/azure_test.go(3 hunks)tests/core-providers/bedrock_test.go(2 hunks)tests/core-providers/cerebras_test.go(3 hunks)tests/core-providers/cohere_test.go(3 hunks)tests/core-providers/config/account.go(17 hunks)tests/core-providers/cross_provider_test.go(1 hunks)tests/core-providers/custom_test.go(1 hunks)tests/core-providers/gemini_test.go(3 hunks)tests/core-providers/groq_test.go(3 hunks)tests/core-providers/mistral_test.go(3 hunks)tests/core-providers/ollama_test.go(3 hunks)tests/core-providers/openai_test.go(3 hunks)tests/core-providers/openrouter_test.go(2 hunks)tests/core-providers/parasail_test.go(3 hunks)tests/core-providers/scenarios/automatic_function_calling.go(4 hunks)tests/core-providers/scenarios/chat_completion_stream.go(8 hunks)tests/core-providers/scenarios/complete_end_to_end.go(2 hunks)tests/core-providers/scenarios/cross_provider_scenarios.go(1 hunks)tests/core-providers/scenarios/embedding.go(3 hunks)tests/core-providers/scenarios/end_to_end_tool_calling.go(2 hunks)tests/core-providers/scenarios/image_base64.go(3 hunks)tests/core-providers/scenarios/image_url.go(2 hunks)tests/core-providers/scenarios/multi_turn_conversation.go(2 hunks)tests/core-providers/scenarios/multiple_images.go(2 hunks)tests/core-providers/scenarios/multiple_tool_calls.go(4 hunks)tests/core-providers/scenarios/reasoning.go(4 hunks)tests/core-providers/scenarios/response_validation.go(10 hunks)tests/core-providers/scenarios/responses_stream.go(6 hunks)tests/core-providers/scenarios/simple_chat.go(3 hunks)tests/core-providers/scenarios/speech_synthesis.go(4 hunks)tests/core-providers/scenarios/speech_synthesis_stream.go(6 hunks)tests/core-providers/scenarios/test_retry_framework.go(4 hunks)tests/core-providers/scenarios/text_completion.go(2 hunks)tests/core-providers/scenarios/text_completion_stream.go(10 hunks)tests/core-providers/scenarios/tool_calls.go(3 hunks)tests/core-providers/scenarios/transcription.go(5 hunks)tests/core-providers/scenarios/transcription_stream.go(6 hunks)tests/core-providers/scenarios/validation_presets.go(7 hunks)tests/core-providers/sgl_test.go(3 hunks)tests/core-providers/vertex_test.go(3 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_edge_cases_test.go
✅ Files skipped from review due to trivial changes (2)
- plugins/telemetry/version
- plugins/otel/version
🚧 Files skipped from review as they are similar to previous changes (30)
- tests/core-providers/scenarios/transcription_stream.go
- core/bifrost.go
- plugins/semanticcache/version
- .github/workflows/scripts/release-core.sh
- plugins/mocker/version
- core/version
- plugins/semanticcache/plugin_responses_test.go
- tests/core-providers/cohere_test.go
- plugins/governance/version
- tests/core-providers/groq_test.go
- tests/core-providers/cerebras_test.go
- core/schemas/bifrost.go
- tests/core-providers/scenarios/text_completion_stream.go
- tests/core-providers/openai_test.go
- framework/version
- tests/core-providers/vertex_test.go
- tests/core-providers/cross_provider_test.go
- plugins/maxim/changelog.md
- plugins/jsonparser/changelog.md
- tests/core-providers/scenarios/cross_provider_scenarios.go
- transports/version
- plugins/logging/changelog.md
- plugins/logging/version
- plugins/jsonparser/version
- tests/core-providers/ollama_test.go
- framework/changelog.md
- tests/core-providers/scenarios/automatic_function_calling.go
- transports/changelog.md
- tests/core-providers/scenarios/reasoning.go
- plugins/governance/changelog.md
🧰 Additional context used
🧬 Code graph analysis (9)
tests/core-providers/scenarios/response_validation.go (1)
plugins/mocker/main.go (1)
Latency(126-130)
plugins/semanticcache/test_utils.go (4)
plugins/mocker/main.go (7)
MockRule(77-85)Conditions(89-94)Response(98-104)ResponseTypeSuccess(28-28)SuccessResponse(108-115)MockerConfig(68-73)Init(156-202)plugins/semanticcache/main.go (2)
Plugin(137-144)Init(263-327)core/bifrost.go (2)
Bifrost(34-51)Init(80-194)core/schemas/account.go (1)
Account(53-69)
tests/core-providers/scenarios/speech_synthesis_stream.go (3)
core/schemas/speech.go (1)
BifrostSpeechStreamResponse(107-112)plugins/mocker/main.go (1)
Latency(126-130)tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)
plugins/mocker/main.go (3)
core/schemas/bifrost.go (6)
BifrostContextKey(96-96)RequestType(79-79)ChatCompletionRequest(84-84)ResponsesRequest(86-86)BifrostResponse(213-223)BifrostResponseExtraFields(251-259)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)ResponsesInputMessageRoleAssistant(304-304)ResponsesMessageTypeMessage(263-263)
tests/core-providers/bedrock_test.go (1)
core/schemas/bifrost.go (2)
Fallback(114-117)Bedrock(38-38)
tests/core-providers/scenarios/responses_stream.go (2)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/responses.go (3)
BifrostResponsesStreamResponse(1330-1363)ResponsesMessage(287-299)ResponsesMessageContent(311-314)
tests/core-providers/scenarios/chat_completion_stream.go (3)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/chatcompletions.go (3)
BifrostChatResponse(20-30)ChatStreamResponseChoice(518-520)ChatMessage(336-345)tests/core-providers/scenarios/utils.go (1)
CreateBasicChatMessage(218-225)
tests/core-providers/config/account.go (4)
core/schemas/account.go (1)
AzureKeyConfig(21-25)ui/lib/types/config.ts (1)
AzureKeyConfig(20-24)core/utils.go (1)
Ptr(12-14)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)
tests/core-providers/scenarios/test_retry_framework.go (2)
core/schemas/bifrost.go (1)
BifrostStream(284-291)tests/core-providers/scenarios/test_retry_conditions.go (1)
EmptyResponseCondition(16-16)
🪛 LanguageTool
plugins/otel/changelog.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...his brings down inference overhead to < 1µseconds
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (35)
plugins/maxim/version (1)
1-1: Version bump LGTM.
plugins/maxim/versioncleanly advances to 1.4.15 with no side effects.plugins/telemetry/changelog.md (1)
4-4: Changelog entry looks good.Version bump note is clear and matches the PR objectives.
tests/core-providers/scenarios/complete_end_to_end.go (1)
23-25: LGTM on the SKIP_PARALLEL_TESTS guard.Matches the suite-wide pattern for conditional parallel runs and keeps behavior unchanged when the env flag is unset.
tests/core-providers/custom_test.go (1)
52-54: Thanks for the skip guard.Gracefully skipping when
GROQ_API_KEYis absent keeps CI noise down and makes the intent explicit. Nicely done.tests/core-providers/scenarios/tool_calls.go (1)
25-27: Parallelization guard LGTMConditional t.Parallel via SKIP_PARALLEL_TESTS matches the suite’s pattern. No issues.
tests/core-providers/scenarios/multiple_tool_calls.go (2)
31-34: Parallelization guard LGTMGood use of SKIP_PARALLEL_TESTS to control t.Parallel.
85-86: Nice: consistent fallbacks on both APIsPropagating testConfig.Fallbacks to both Chat and Responses aligns behaviors and reduces split‑brain debugging in dual‑API runs.
Also applies to: 98-99
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
23-25: Parallelization guard LGTMMatches the rest of the scenarios; safe default with opt‑out via env.
tests/core-providers/scenarios/chat_completion_stream.go (3)
24-27: Parallelization guard LGTMBoth streaming subtests respect SKIP_PARALLEL_TESTS; consistent with suite.
Also applies to: 222-224
91-91: Good: defensive copy of last stream chunkDeepCopyBifrostStream avoids pooled/aliased state issues when assembling the consolidated response.
210-211: Non‑fatal validation failure is acceptableSwitch to t.Errorf lets the test report metrics before failing; suits diagnostic workflows.
tests/core-providers/scenarios/test_retry_framework.go (3)
768-778: ReasoningRetryConfig addition: OKConfig looks sane (5 attempts, moderate backoff). No further concerns.
618-629: Backoff calc LGTMExponential backoff with capping is correct; attempt indexing is consistent (attempt-1).
633-666: Retry helpers and dual‑API orchestrator look solidClear separation of configs per modality and a pragmatic dual‑API coordinator. Good logging hooks for retries/final failure.
Also applies to: 686-702, 704-718, 720-750, 752-766, 780-794, 796-916, 918-949, 951-971
tests/core-providers/scenarios/speech_synthesis_stream.go (3)
155-157: Latency log: confirm unit consistency.You're logging “ms” with a raw integer. Ensure
ExtraFields.Latencyis milliseconds across providers/mocker; otherwise, label or convert accordingly.
178-179: Good: snapshotting last stream state.Using
DeepCopyBifrostStreamavoids data races on reused structs during streaming.
230-233: Parallel gating LGTM.Conditional
t.Parallel()behindSKIP_PARALLEL_TESTSis consistent with suite-wide pattern.tests/core-providers/scenarios/responses_stream.go (7)
24-28: Parallel gating LGTM.Consistent use of
SKIP_PARALLEL_TESTSguard.
102-107: Good: DeepCopy avoids shared-state pitfalls.Capturing
lastResponseviaDeepCopyBifrostStreamis the right approach.
113-115: Latency log: verify unit and presence.Logging “ms” assumes milliseconds. Confirm
ExtraFields.Latencyunit; if duration/nanos, convert before logging.
240-243: Parallel gating for Tools subtest LGTM.Matches the suite’s pattern.
248-249: Prompt tweak LGTM.Including “in celsius” clarifies expected tool behavior.
370-373: Parallel gating for Reasoning subtest LGTM.Consistent and safe.
611-619: Nice: final-chunk latency validation.Ensures total latency is present; helpful for perf assertions.
tests/core-providers/scenarios/text_completion.go (1)
21-25: Parallel gating LGTM.Simple, consistent with other scenarios.
tests/core-providers/scenarios/simple_chat.go (2)
21-25: Parallel gating LGTM.Consistent with the suite.
100-110: Good: propagate Fallbacks to Responses API.Keeps behavior parity with other request types.
tests/core-providers/scenarios/embedding.go (1)
51-55: Parallel gating LGTM.No issues.
plugins/semanticcache/changelog.md (1)
4-5: LGTM.Changelog accurately reflects the changes.
plugins/semanticcache/test_utils.go (1)
300-323: Plugin wiring order is good.semanticcache first, then mocker ensures cache can capture mocked responses. LGTM.
plugins/mocker/main.go (1)
3-19: No changes needed: go.mod uses Go 1.24.1, which supports maps.Copy and slices.Contains.tests/core-providers/anthropic_test.go (1)
13-15: Good env guard.Clear skip prevents flaky CI when ANTHROPIC_API_KEY is absent.
tests/core-providers/parasail_test.go (1)
13-15: Good env guard.Prevents accidental live calls when PARASAIL_API_KEY is missing.
tests/core-providers/mistral_test.go (1)
13-15: Good env guard.Skip condition avoids noisy CI noise when MISTRAL_API_KEY isn’t set.
tests/core-providers/gemini_test.go (1)
13-15: LGTM on env guard.Consistent skip message and check.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/core-providers/scenarios/transcription.go (1)
164-166: Don’t ignore GenerateTTSAudioForTest errors and fix loop-var capture in parallel subtests
- Replace every discarded error on GenerateTTSAudioForTest (tests/core-providers/scenarios/transcription.go lines 164, 203, 246, 274; transcription_stream.go lines 316, 412, 507) with
audioData, err := GenerateTTSAudioForTest(...) require.NoError(t, err, "failed to generate test audio")- In the
for _, tc := range customCasesloop (transcription.go 159–166), addimmediately beforetc := tct.Run(tc.name, …)to avoid closing over the sametcin all parallel runs.tests/core-providers/scenarios/embedding.go (1)
104-123: Fix invalid loop and 2D indexing before merging.The new loop
for i := range responseDataLengthwill not compile—rangeexpects an array/slice/map/string/channel, not anint. Even after switching to a traditionalfor, the current index path still panics for 2D embeddings: onceresponseDataLengthis overwritten withlen(response.Data[0].Embedding.Embedding2DArray), subsequent iterations accessresponse.Data[i]wherelen(response.Data)may still be 1.Please restore a plain indexed loop and branch on 1D vs 2D payloads so we only index
response.Datawhen it actually has that many elements. For example:- for i := range responseDataLength { - vec, extractErr := getEmbeddingVector(response.Data[i]) + is2D := responseDataLength != len(response.Data) + for i := 0; i < responseDataLength; i++ { + var ( + vec []float32 + extractErr error + ) + if !is2D { + if i >= len(response.Data) { + t.Fatalf("Embedding response missing data at index %d", i) + } + vec, extractErr = getEmbeddingVector(response.Data[i]) + } else { + vec, extractErr = getEmbeddingVectorFrom2D(response.Data[0], i) + } if extractErr != nil { t.Fatalf("Failed to extract embedding vector for text '%s': %v", testTexts[i], extractErr) }…with
getEmbeddingVectorFrom2Dimplemented (or reused if it already exists) to safely read the ith row from the 2D array. This addresses both the compile-time failure and the runtime panic risk.docs/features/plugins/mocker.mdx (1)
33-39: Fix variable name in error check.Use initErr when checking Init error.
- client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ + client, initErr := bifrost.Init(context.Background(), schemas.BifrostConfig{ Account: &yourAccount, Plugins: []schemas.Plugin{plugin}, }) - if err != nil { - panic(err) + if initErr != nil { + panic(initErr) }tests/core-providers/azure_test.go (1)
28-29: Handle deprecated embedding model in Azure tests
- Replace
EmbeddingModel: "text-embedding-ada-002"with"text-embedding-3-small"(or large) in tests/core-providers/azure_test.go.- Extend the Deployments map in tests/core-providers/config/account.go to include entries for
"text-embedding-3-small"(and"text-embedding-3-large") so the new models resolve correctly.- Alternatively, surface EmbeddingModel as an env-configurable value (fallback to ada-002) to ease future upgrades.
plugins/mocker/main.go (2)
858-861: Nil panic: overriding ChatResponse.Model even for ResponsesRequest.When mocking a ResponsesRequest, ChatResponse is nil; this dereferences nil.
- // Override model if specified - if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + // Override model if specified + if content.Model != nil { + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + mockResponse.ChatResponse.ExtraFields.ModelRequested = *content.Model + } else if mockResponse.ResponsesResponse != nil { + mockResponse.ResponsesResponse.ExtraFields.ModelRequested = *content.Model + } + }
667-686: Nil-guard chat message Content in fast extract/size paths.ChatMessage.Content can be nil; add checks to avoid panics.
@@ - // Fast path for single message - if len(messages) == 1 { - if messages[0].Content.ContentStr != nil { - return *messages[0].Content.ContentStr - } - return "" - } + // Fast path for single message + if len(messages) == 1 { + if messages[0].Content != nil && messages[0].Content.ContentStr != nil { + return *messages[0].Content.ContentStr + } + return "" + } @@ - for i, message := range messages { - if message.Content.ContentStr != nil { + for i, message := range messages { + if message.Content != nil && message.Content.ContentStr != nil { if i > 0 { builder.WriteByte(' ') } builder.WriteString(*message.Content.ContentStr) } } @@ - if req.ChatRequest.Input != nil { - for _, message := range req.ChatRequest.Input { - if message.Content.ContentStr != nil { - size += len(*message.Content.ContentStr) - } - size += 50 // Approximate overhead for message structure - } - } + if req.ChatRequest.Input != nil { + for _, message := range req.ChatRequest.Input { + if message.Content != nil && message.Content.ContentStr != nil { + size += len(*message.Content.ContentStr) + } + size += 50 // Approximate overhead for message structure + } + }Also applies to: 741-748
♻️ Duplicate comments (1)
tests/core-providers/cohere_test.go (1)
38-40: Update the vision-support comment.The inline note still points to “c4ai-aya-vision-8b,” but this scenario is exercising
command-a-vision-07-2025. Please align the comment with the model actually under test (or remove it if it no longer adds value).
🧹 Nitpick comments (16)
transports/changelog.md (1)
6-6: Clarify microsecond notation for readability.Suggest formatting the unit as “< 1 µs” (or “< 1 microsecond”) to match common SI style and avoid the unusual pluralization “µseconds.”
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
23-25: HandleSKIP_PARALLEL_TESTSparsing more defensively.Right now the guard only trips when the variable is exactly the lowercase string
"true". That’s brittle—CI configs and local shells often set booleans asTRUE,True, or1, which would cause us to run in parallel when parallelism was meant to be disabled. Please feed the env var throughstrconv.ParseBool(or at least a case-insensitive comparison) so all common truthy spellings are honored.Consider:
-import "os" +import ( + "os" + "strconv" +) - if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + skipParallel, _ := strconv.ParseBool(os.Getenv("SKIP_PARALLEL_TESTS")) + if !skipParallel { t.Parallel() }tests/core-providers/scenarios/multiple_images.go (1)
23-25: Make the SKIP_PARALLEL_TESTS guard robust to common truthy values.Accept "1/true/TRUE" to avoid accidental parallelism in CI.
Apply:
- if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + val := os.Getenv("SKIP_PARALLEL_TESTS") + if !strings.EqualFold(val, "true") && val != "1" { t.Parallel() - } + }tests/core-providers/scenarios/multiple_tool_calls.go (1)
31-33: Broaden env check for SKIP_PARALLEL_TESTS.Handle "1/true/TRUE" to disable parallelism reliably.
- if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + val := os.Getenv("SKIP_PARALLEL_TESTS") + if !strings.EqualFold(val, "true") && val != "1" { t.Parallel() - } + }tests/core-providers/scenarios/transcription.go (1)
241-244: Broaden env check for SKIP_PARALLEL_TESTS.- if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + val := os.Getenv("SKIP_PARALLEL_TESTS") + if !strings.EqualFold(val, "true") && val != "1" { t.Parallel() - } + }tests/core-providers/scenarios/test_retry_framework.go (1)
619-629: Prefer integer shifting over float math for backoff.Avoid float conversion/rounding for durations.
-func calculateRetryDelay(attempt int, baseDelay, maxDelay time.Duration) time.Duration { - // Exponential backoff: baseDelay * 2^attempt - delay := time.Duration(float64(baseDelay) * math.Pow(2, float64(attempt))) +func calculateRetryDelay(attempt int, baseDelay, maxDelay time.Duration) time.Duration { + // Exponential backoff: baseDelay << attempt + if attempt >= 0 && attempt < 62 { + delay := baseDelay << uint(attempt) + if delay > maxDelay { + return maxDelay + } + return delay + } - // Cap at maximum delay - if delay > maxDelay { - delay = maxDelay - } - - return delay + return maxDelay }tests/core-providers/scenarios/image_url.go (1)
23-25: Harden SKIP_PARALLEL_TESTS parsing.Accept common truthy values to reliably disable parallelism.
- if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + val := os.Getenv("SKIP_PARALLEL_TESTS") + if !strings.EqualFold(val, "true") && val != "1" { t.Parallel() - } + }tests/core-providers/scenarios/validation_presets.go (1)
464-466: Bool merge semantics prevent disabling latency via CombineExpectationsCombineExpectations only assigns when exp.ShouldHaveLatency is true, so you cannot override a base=true to false via merging. This mirrors other bools here, but with latency now defaulted to true globally, scenario-level opt-outs (if ever needed) won’t work through CombineExpectations.
Options:
- Introduce tri-state fields (e.g., *bool) for mergeable flags.
- Or add explicit “Override” helpers for selective fields (e.g., WithLatency(required bool)) invoked before/after merges.
- Short term: ensure any provider/scenario needing to disable latency does so outside CombineExpectations (direct struct edit), as done in ModifyExpectationsForProvider for other flags.
tests/core-providers/bedrock_test.go (1)
51-54: Subtest + explicit shutdown LGTM; consider t.Cleanup.Optional: t.Cleanup(func(){ client.Shutdown() }) to guarantee cleanup even on subtest panic.
docs/features/plugins/mocker.mdx (2)
110-127: Unify pointer helpers in examples.Use a single helper (e.g., bifrost.Ptr) or show a stringPtr/boolPtr helper definition to avoid confusion.
- MessageRegex: stringPtr("(?i).*hello.*"), + MessageRegex: bifrost.Ptr("(?i).*hello.*"),- Code: stringPtr("429"), + Code: bifrost.Ptr("429"),- MessageRegex: stringPtr(`(?i).*support.*|.*help.*`), + MessageRegex: bifrost.Ptr(`(?i).*support.*|.*help.*`),Also applies to: 240-243, 271-274
193-201: Add missing context import in snippet.Snippet uses context.WithValue; include import for clarity.
-import "github.com/maximhq/bifrost/core/schemas" +import ( + "context" + "github.com/maximhq/bifrost/core/schemas" +)tests/core-providers/azure_test.go (1)
4-4: <!-- [scratchpad_start]
[task review_file_1/1]
State and verify suggestions from original comment.
[observations]
- Test already guards on AZURE_OPENAI_ENDPOINT (lines 13–16) and skips if unset.
- CI environment shows AZURE_OPENAI_ENDPOINT is missing, so guard works as intended.
- Subtest teardown at lines 46–49 uses manual shutdown; could use t.Cleanup.
[analysis]- Verifying endpoint var is redundant since guard exists.
- t.Cleanup suggestion remains a valid optional refactor.
[pending]
None.
[actions]- Remove endpoint-verification recommendation.
- Keep t.Cleanup suggestion.
[done]
Ready to emit rewritten review comment.
[scratchpad_end]-->Use t.Cleanup for teardown
Replace the manualclient.Shutdown()in your subtest witht.Cleanup(func() { client.Shutdown() })to guarantee cleanup even if the subtest panics.plugins/semanticcache/test_utils.go (2)
137-144: Broaden AI prompt regex to match “Explain artificial intelligence”.Covers prompts used in tests without falling back to the default rule.
- Conditions: mocker.Conditions{MessageRegex: bifrost.Ptr("(?i)what is artificial intelligence\\?|can you explain what ai is\\?|define artificial intelligence")}, + Conditions: mocker.Conditions{MessageRegex: bifrost.Ptr("(?i)(what is artificial intelligence\\?|can you explain what ai is\\?|define artificial intelligence|explain artificial intelligence)")},
356-361: Update comment: mocker now covers Responses, not just chat.Tighten wording to prevent future confusion.
- // Keep Weaviate for embeddings, as mocker only affects chat completions + // Keep Weaviate for embeddings; mocker affects chat/responses only (not embeddings)plugins/mocker/main.go (2)
495-503: Name the context key to avoid stringly-typed access.Define a typed const and use it here.
+ // Well-known context key to skip mocker per request + const SkipMockerContextKey schemas.BifrostContextKey = "skip-mocker" @@ - skipMocker, ok := (*ctx).Value(schemas.BifrostContextKey("skip-mocker")).(bool) + skipMocker, ok := (*ctx).Value(SkipMockerContextKey).(bool)
981-1030: DefaultBehaviorSuccess returns ChatResponse for all request types.For non-chat requests this is inconsistent. Consider matching the incoming RequestType or keeping passthrough as the only default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
.github/workflows/scripts/release-core.sh(1 hunks)core/bifrost.go(1 hunks)core/changelog.md(1 hunks)core/schemas/bifrost.go(1 hunks)core/version(1 hunks)docs/architecture/core/concurrency.mdx(1 hunks)docs/features/plugins/mocker.mdx(5 hunks)framework/changelog.md(1 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/main.go(6 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(0 hunks)plugins/semanticcache/plugin_responses_test.go(3 hunks)plugins/semanticcache/test_utils.go(4 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/anthropic_test.go(3 hunks)tests/core-providers/azure_test.go(3 hunks)tests/core-providers/bedrock_test.go(2 hunks)tests/core-providers/cerebras_test.go(3 hunks)tests/core-providers/cohere_test.go(3 hunks)tests/core-providers/config/account.go(17 hunks)tests/core-providers/cross_provider_test.go(1 hunks)tests/core-providers/custom_test.go(1 hunks)tests/core-providers/gemini_test.go(3 hunks)tests/core-providers/groq_test.go(3 hunks)tests/core-providers/mistral_test.go(3 hunks)tests/core-providers/ollama_test.go(3 hunks)tests/core-providers/openai_test.go(3 hunks)tests/core-providers/openrouter_test.go(2 hunks)tests/core-providers/parasail_test.go(3 hunks)tests/core-providers/scenarios/automatic_function_calling.go(4 hunks)tests/core-providers/scenarios/chat_completion_stream.go(8 hunks)tests/core-providers/scenarios/complete_end_to_end.go(2 hunks)tests/core-providers/scenarios/cross_provider_scenarios.go(1 hunks)tests/core-providers/scenarios/embedding.go(3 hunks)tests/core-providers/scenarios/end_to_end_tool_calling.go(2 hunks)tests/core-providers/scenarios/image_base64.go(3 hunks)tests/core-providers/scenarios/image_url.go(2 hunks)tests/core-providers/scenarios/multi_turn_conversation.go(2 hunks)tests/core-providers/scenarios/multiple_images.go(2 hunks)tests/core-providers/scenarios/multiple_tool_calls.go(4 hunks)tests/core-providers/scenarios/reasoning.go(4 hunks)tests/core-providers/scenarios/response_validation.go(10 hunks)tests/core-providers/scenarios/responses_stream.go(6 hunks)tests/core-providers/scenarios/simple_chat.go(3 hunks)tests/core-providers/scenarios/speech_synthesis.go(4 hunks)tests/core-providers/scenarios/speech_synthesis_stream.go(6 hunks)tests/core-providers/scenarios/test_retry_framework.go(4 hunks)tests/core-providers/scenarios/text_completion.go(2 hunks)tests/core-providers/scenarios/text_completion_stream.go(10 hunks)tests/core-providers/scenarios/tool_calls.go(3 hunks)tests/core-providers/scenarios/transcription.go(5 hunks)tests/core-providers/scenarios/transcription_stream.go(6 hunks)tests/core-providers/scenarios/validation_presets.go(7 hunks)tests/core-providers/sgl_test.go(3 hunks)tests/core-providers/vertex_test.go(3 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_edge_cases_test.go
✅ Files skipped from review due to trivial changes (4)
- tests/core-providers/scenarios/cross_provider_scenarios.go
- core/version
- plugins/logging/changelog.md
- plugins/mocker/version
🚧 Files skipped from review as they are similar to previous changes (26)
- core/schemas/bifrost.go
- tests/core-providers/openai_test.go
- framework/version
- plugins/otel/version
- plugins/jsonparser/version
- tests/core-providers/scenarios/simple_chat.go
- tests/core-providers/scenarios/transcription_stream.go
- plugins/governance/version
- core/bifrost.go
- plugins/maxim/version
- plugins/maxim/changelog.md
- framework/changelog.md
- plugins/telemetry/changelog.md
- plugins/logging/version
- tests/core-providers/scenarios/reasoning.go
- plugins/semanticcache/version
- tests/core-providers/config/account.go
- tests/core-providers/groq_test.go
- tests/core-providers/mistral_test.go
- .github/workflows/scripts/release-core.sh
- tests/core-providers/scenarios/automatic_function_calling.go
- tests/core-providers/sgl_test.go
- plugins/governance/changelog.md
- core/changelog.md
- tests/core-providers/cross_provider_test.go
- transports/version
🧰 Additional context used
🧬 Code graph analysis (9)
tests/core-providers/scenarios/text_completion_stream.go (2)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/textcompletions.go (1)
BifrostTextCompletionResponse(64-72)
tests/core-providers/scenarios/chat_completion_stream.go (3)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/chatcompletions.go (3)
BifrostChatResponse(20-30)ChatStreamResponseChoice(518-520)ChatMessage(336-345)tests/core-providers/scenarios/utils.go (1)
CreateBasicChatMessage(218-225)
tests/core-providers/scenarios/response_validation.go (1)
plugins/mocker/main.go (1)
Latency(126-130)
plugins/semanticcache/test_utils.go (4)
plugins/mocker/main.go (7)
MockRule(77-85)Conditions(89-94)Response(98-104)ResponseTypeSuccess(28-28)SuccessResponse(108-115)MockerConfig(68-73)Init(156-202)plugins/semanticcache/main.go (2)
Plugin(137-144)Init(263-327)core/bifrost.go (2)
Bifrost(34-51)Init(80-194)core/schemas/account.go (1)
Account(53-69)
plugins/mocker/main.go (3)
core/schemas/bifrost.go (6)
BifrostContextKey(96-96)RequestType(79-79)ChatCompletionRequest(84-84)ResponsesRequest(86-86)BifrostResponse(213-223)BifrostResponseExtraFields(251-259)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)ResponsesInputMessageRoleAssistant(304-304)ResponsesMessageTypeMessage(263-263)
tests/core-providers/bedrock_test.go (1)
core/schemas/bifrost.go (2)
Fallback(114-117)Bedrock(38-38)
tests/core-providers/scenarios/speech_synthesis_stream.go (2)
core/schemas/speech.go (1)
BifrostSpeechStreamResponse(107-112)tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)
tests/core-providers/scenarios/responses_stream.go (2)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/responses.go (4)
BifrostResponsesStreamResponse(1330-1363)ResponsesMessage(287-299)ResponsesInputMessageRoleUser(305-305)ResponsesMessageContent(311-314)
tests/core-providers/scenarios/test_retry_framework.go (2)
core/schemas/bifrost.go (1)
BifrostStream(284-291)tests/core-providers/scenarios/test_retry_conditions.go (1)
EmptyResponseCondition(16-16)
🪛 LanguageTool
plugins/otel/changelog.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...his brings down inference overhead to < 1µseconds
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (22)
plugins/semanticcache/changelog.md (1)
4-5: Changelog entry reads wellVersion bump and testing note are clear and consistent with the PR scope.
plugins/jsonparser/changelog.md (1)
4-4: Changelog entry looks consistentMatches the core 1.2.13 and framework 1.1.15 versions referenced elsewhere in the stack.
tests/core-providers/scenarios/tool_calls.go (1)
25-27: Parallel guard aligns with env toggle. HonoringSKIP_PARALLEL_TESTSbefore callingt.Parallel()keeps this scenario consistent with the rest of the suite while retaining parallel execution by default. Nicely done.tests/core-providers/scenarios/image_base64.go (1)
23-25: Parallel guard honors env toggle. Matches the rest of the suite by defaulting to parallel runs yet respectingSKIP_PARALLEL_TESTS=truefor serial execution.tests/core-providers/scenarios/complete_end_to_end.go (1)
22-26: Parallel gating looks great.Thanks for matching the existing SKIP_PARALLEL_TESTS pattern—this keeps the fast default while providing a clean escape hatch when the environment needs sequential runs.
docs/architecture/core/concurrency.mdx (1)
271-295: Diagram update clarifies pool lifecycle
The added PoolCheck decision and bidirectional NewObject/UseObject path make the reuse vs factory flow much easier to parse, and the GC Interaction tweak reads well. Nicely done.tests/core-providers/scenarios/multi_turn_conversation.go (1)
23-25: LGTM on conditional parallelization. The gatedt.Parallel()keeps the suite fast by default while still offering an opt-out whenSKIP_PARALLEL_TESTS=true. Nicely aligned with the broader test updates.tests/core-providers/gemini_test.go (2)
43-43: Confirm disabling multi-image scenario.Turning
MultipleImagesoff means we no longer exercise the multi-image path in the comprehensive suite. Unless the Gemini provider truly can’t handle multi-image input (or there’s a known blocking bug), this drops coverage for a supported capability. Please confirm the reason and either keep the scenario enabled or add a TODO referencing the blocker so we don’t lose sight of it.
52-56: Subtest wrapper and explicit shutdown look good.Wrapping the comprehensive run in a named subtest and invoking
client.Shutdown()explicitly keeps the flow consistent with the other provider suites and ensures orderly cleanup.tests/core-providers/scenarios/multiple_tool_calls.go (1)
85-86: Fallbacks propagation LGTM.Passing testConfig.Fallbacks into both Chat and Responses requests aligns scenarios and improves parity.
Also applies to: 98-99
tests/core-providers/scenarios/validation_presets.go (5)
114-115: Embeddings now require latency — looks goodMaking embeddings latency-aware aligns with PR goals. Ensure the validator checks latency presence in the embedding response path (e.g., via ExtraFields/Metrics) so this expectation is actually asserted.
165-165: Speech presets: latency requiredLGTM. Please confirm audio-generation paths populate latency consistently across providers to avoid flaky assertions.
184-184: Transcription presets: latency requiredLGTM. Verify transcription validators read latency from the same source as chat/speech to keep checks uniform.
210-213: Reasoning defaults expanded with "solve"Good addition; broadens acceptable reasoning markers without overfitting.
25-25: Latency instrumentation is universal
Confirmed that every provider path measures and assignsExtraFields.Latency(including error and cancellation cases viamakeRequestWithContext) and no test disables latency checks. GlobalShouldHaveLatency = trueis safe.tests/core-providers/ollama_test.go (1)
4-4: LGTM on guard/subtest; config aligns with Ollama capabilities.Also applies to: 13-16, 45-49
plugins/mocker/changelog.md (1)
4-6: Changelog entries look good.tests/core-providers/anthropic_test.go (1)
4-4: Verify resolver supports new model names
No occurrences ofclaude-sonnet-4-20250514orclaude-3-7-sonnet-20250219were found in the core code. Please confirm your resolver accepts these model strings.tests/core-providers/cerebras_test.go (1)
4-4: Confirm Cerebras model names
“llama-3.3-70b” and “llama3.1-8b” aren’t defined in the provider mappings—please manually verify these match the current Cerebras API model identifiers (tests/core-providers/cerebras_test.go:25-26).tests/core-providers/parasail_test.go (1)
4-4: Confirm Parasail model key
Verify that"parasail-deepseek-r1"exactly matches the model identifier supported by the Parasail provider implementation.plugins/semanticcache/test_utils.go (1)
312-317: Plugin order LGTM (semantic cache before mocker).Ensures cache PreHook sees requests before mock short‑circuits; good for cache population and streaming fallback.
plugins/mocker/main.go (1)
9-9: Stdlib slices/maps supported (Go ≥1.21 confirmed)
plugins/mocker/go.mod specifiesgo 1.24.1; bothslicesandmapsimports are valid.
akshaydeo
force-pushed
the
10-22-chore_tests_fixes
branch
from
414f985 to
bff252b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/core-providers/scenarios/transcription.go (2)
208-236: Fix loop variable capture and useformatCopyconsistently.Two issues here:
- The loop variable
formatis not captured before the closure, causing a race on the test name and error messages.- Although
formatCopyis created inside the closure, the originalformatvariable is still used in error messages (lines 229-230), defeating the purpose of the copy.Apply this diff:
for _, format := range formats { + format := format // capture for parallel subtests t.Run("Format_"+format, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() } formatCopy := format request := &schemas.BifrostTranscriptionRequest{ Provider: testConfig.Provider, Model: testConfig.TranscriptionModel, Input: &schemas.TranscriptionInput{ File: audioData, }, Params: &schemas.TranscriptionParameters{ Format: bifrost.Ptr("mp3"), ResponseFormat: &formatCopy, }, Fallbacks: testConfig.Fallbacks, } response, err := client.TranscriptionRequest(ctx, request) - require.Nilf(t, err, "Transcription failed for format %s: %v", format, err) - require.NotNil(t, response, "Transcription returned nil response for format %s", format) + require.Nilf(t, err, "Transcription failed for format %s: %v", formatCopy, err) + require.NotNil(t, response, "Transcription returned nil response for format %s", formatCopy) // All formats should return some text assert.NotEmpty(t, response.Text) - t.Logf("✅ Format %s successful: '%s'", format, response.Text) + t.Logf("✅ Format %s successful: '%s'", formatCopy, response.Text) }) }Based on past review comments.
279-304: Fix loop variable capture and uselangCopyconsistently.Same issues as the formats loop:
- The loop variable
langis not captured before the closure.- Although
langCopyis created, the originallangvariable is still used in error messages and logs (lines 300-301, 303).Apply this diff:
for _, lang := range languages { + lang := lang // capture for parallel subtests t.Run("Language_"+lang, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() } langCopy := lang request := &schemas.BifrostTranscriptionRequest{ Provider: testConfig.Provider, Model: testConfig.TranscriptionModel, Input: &schemas.TranscriptionInput{ File: audioData, }, Params: &schemas.TranscriptionParameters{ Format: bifrost.Ptr("mp3"), Language: &langCopy, }, Fallbacks: testConfig.Fallbacks, } response, err := client.TranscriptionRequest(ctx, request) - require.Nilf(t, err, "Transcription failed for language %s: %v", lang, err) - require.NotNil(t, response, "Transcription returned nil response for language %s", lang) + require.Nilf(t, err, "Transcription failed for language %s: %v", langCopy, err) + require.NotNil(t, response, "Transcription returned nil response for language %s", langCopy) assert.NotEmpty(t, response.Text) - t.Logf("✅ Language %s transcription successful: '%s'", lang, response.Text) + t.Logf("✅ Language %s transcription successful: '%s'", langCopy, response.Text) }) }Based on past review comments.
plugins/mocker/main.go (1)
857-860: Fix potential nil pointer dereference for ResponsesRequest.Lines 857-860 unconditionally access
mockResponse.ChatResponse.Model, but forResponsesRequesttypes,ChatResponsewill be nil, causing a panic ifcontent.Modelis set.Apply this diff to guard the model override:
// Override model if specified - if content.Model != nil { + if content.Model != nil && req.RequestType == schemas.ChatCompletionRequest { mockResponse.ChatResponse.Model = *content.Model }Alternatively, handle both request types:
// Override model if specified if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + } + // Note: ResponsesResponse doesn't have a top-level Model field to override }
♻️ Duplicate comments (13)
plugins/otel/changelog.md (2)
5-5: Pluralize "Value" for grammatical correctness.Line 5 should read "Values prefixed with env" or be clarified further (e.g., "Configuration values prefixed with env…") for proper grammar and clarity.
[suggest_minor_issue]Apply this diff:
-- feat: added headers support for OTel configuration. Value prefixed with env will be fetched from environment variables (env.<ENV_VAR_NAME>) +- feat: added headers support for OTel configuration. Values prefixed with env will be fetched from environment variables (env.<ENV_VAR_NAME>)
6-6: Fix microsecond unit formatting.Line 6 uses "µseconds" which is not a standard unit. Replace with "µs" (microseconds symbol) or spell out "microsecond(s)" for clarity.
[suggest_minor_issue]Apply this diff:
-- feat: emission of OTel resource spans is completely async - this brings down inference overhead to < 1µseconds +- feat: emission of OTel resource spans is completely async - this brings down inference overhead to < 1 µsdocs/architecture/core/concurrency.mdx (1)
271-296: Restore the pool recursion edge (previous issue remains unresolved).The pool lifecycle diagram is incomplete: it shows the path from
GetObjectthroughPoolCheck,NewObject,UseObject,ResetObject, toReturnObject, but lacks the critical edge showing that returned objects re-enter the pool for reuse. WithoutReturnObject --> GetObject, the diagram fails to communicate the cyclic nature of object pooling—a core semantic feature ofsync.Pool.Apply this diff to close the lifecycle loop:
GetObject --> PoolCheck PoolCheck -- Yes --> NewObject PoolCheck -- No --> UseObject NewObject --> UseObject UseObject --> ResetObject ResetObject --> ReturnObject + ReturnObject --> GetObject endThis edge is essential for the diagram to accurately represent the pool's reuse mechanism, where returned objects become available for subsequent
GetObjectrequests.tests/core-providers/custom_test.go (1)
44-47: Restore panic-safe shutdown (duplicate concern).The explicit
client.Shutdown()on line 47 won't execute ifrunAllComprehensiveTestspanics, leaving background workers and connections active for the remainder of the test suite. This is inconsistent withTestCustomProvider_DisallowedOperation(line 61) andTestCustomProvider_MismatchedIdentity(line 99), both of which correctly usedefer client.Shutdown().Apply this diff to ensure shutdown always runs:
+ t.Cleanup(func() { client.Shutdown() }) t.Run("CustomProviderTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/scenarios/embedding.go (1)
104-123: Critical compile-time error remains unaddressed.The issues flagged in the previous review are still present:
Line 114:
for i := range responseDataLengthis invalid Go syntax—you cannot userangeon aninttype. This will fail at compile time.Line 115: When
responseDataLengthis derived fromEmbedding2DArray(line 107),response.Datahas only one element, soresponse.Data[i]will panic wheni >= 1.The previous review provided a complete fix with a helper function to handle 2D embeddings. Please apply that fix to resolve both issues.
tests/core-providers/scenarios/speech_synthesis_stream.go (2)
346-429: Fix loop-var capture in voice iteration before t.Parallel().The closure reads the loop variable
voicedirectly in multiple locations (test name at line 347, retry context at lines 370 & 377, error/log messages at lines 385, 401, 406, 412, 416, 423, 427, 429), causing race conditions whent.Parallel()executes. AlthoughvoiceCopyis created on line 352, it's only used in the request and doesn't protect the other references.Apply this diff to capture
voicebeforet.Runand use the captured copy everywhere:for _, voice := range voices { - t.Run("StreamingVoice_"+voice, func(t *testing.T) { + v := voice // capture loop var + t.Run("StreamingVoice_"+v, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() } - voiceCopy := voice + voiceCopy := v request := &schemas.BifrostSpeechRequest{ // ... (request body unchanged) } retryContext := TestRetryContext{ - ScenarioName: "SpeechSynthesisStream_Voice_" + voice, + ScenarioName: "SpeechSynthesisStream_Voice_" + v, ExpectedBehavior: map[string]interface{}{ "generate_streaming_audio": true, - "voice_type": voice, + "voice_type": v, }, TestMetadata: map[string]interface{}{ "provider": testConfig.Provider, - "voice": voice, + "voice": v, }, } responseChannel, err := WithStreamRetry(t, retryConfig, retryContext, func() (chan *schemas.BifrostStream, *schemas.BifrostError) { return client.SpeechStreamRequest(ctx, request) }) - RequireNoError(t, err, fmt.Sprintf("Streaming failed for voice %s", voice)) + RequireNoError(t, err, fmt.Sprintf("Streaming failed for voice %s", v)) // ... (inside loop, replace all voice references with v) // Lines 401, 406, 412, 416, 423, 427, 429: replace voice with vAlso replace all remaining
voicereferences withvin error messages and logs within the subtest closure (lines 401, 406, 412, 416, 423, 427, 429).
63-67: Fix loop-var capture before t.Parallel() to avoid flaky subtests.The closure captures the
tcloop variable, which will cause race conditions and incorrect test data whent.Parallel()executes. Each parallel subtest may observe mutatedtcvalues from later iterations.Apply this diff to capture the loop variable per-iteration:
for _, tc := range testCases { + tc := tc // capture loop var t.Run(tc.name, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() }tests/core-providers/scenarios/transcription.go (2)
57-63: Fix loop variable capture for parallel subtests.The loop variable
tcis captured by thet.Runclosure while tests may run in parallel, causing race conditions. Each parallel subtest may see a mutated or final value oftcinstead of its intended test case.Apply this diff to capture the loop variable before the closure:
for _, tc := range roundTripCases { + tc := tc // capture for parallel subtests t.Run(tc.name, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() }Based on past review comments.
157-161: Capture loop variable for parallel subtests.Same race condition as the
roundTripCasesloop:tcmust be captured before thet.Runclosure to avoid data races when tests run in parallel.Apply this diff:
for _, tc := range customCases { + tc := tc // capture for parallel subtests t.Run(tc.name, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() }Based on past review comments.
tests/core-providers/config/account.go (1)
366-366: Critical: Ollama BaseURL still missing default value.This issue was flagged in a previous review but remains unaddressed. The BaseURL is set directly from
os.Getenv("OLLAMA_BASE_URL")with no default, which will result in an empty string if the environment variable is unset, causing Ollama requests to fail.Apply the previously suggested fix:
- BaseURL: os.Getenv("OLLAMA_BASE_URL"), + BaseURL: getEnvWithDefault("OLLAMA_BASE_URL", "http://localhost:11434"),Alternatively, add test gating to skip Ollama tests when
OLLAMA_BASE_URLis not set (similar to the AWS credentials check in bedrock_test.go).tests/core-providers/bedrock_test.go (1)
25-29: Add AWS_REGION guard and verify model ID consistency.The model IDs are consistent with the config/account.go updates:
ChatModeluses direct Bedrock IDReasoningModeluses friendly name requiring deployment mappingEmbeddingModelmatches the second Bedrock keyHowever, the test still lacks an AWS_REGION guard (flagged in past review).
Apply this fix to add region validation:
func TestBedrock(t *testing.T) { if os.Getenv("AWS_ACCESS_KEY_ID") == "" || os.Getenv("AWS_SECRET_ACCESS_KEY") == "" { t.Skip("Skipping Bedrock embedding: AWS credentials not set") } + if os.Getenv("AWS_REGION") == "" { + t.Skip("Skipping Bedrock tests: AWS_REGION not set") + } client, ctx, cancel, err := config.SetupTest()tests/core-providers/scenarios/test_retry_framework.go (1)
52-56: Deep copy bug: check/set on destination, not source (skips fields).Use dstField.CanSet() and set dstField; current code gates copy on source.CanSet(), causing partial/zeroed clones. This matches prior feedback.
- for i := 0; i < original.NumField(); i++ { - field := original.Field(i) - if field.CanSet() { - newStruct.Field(i).Set(deepCopyValue(field)) - } - } + for i := 0; i < original.NumField(); i++ { + srcField := original.Field(i) + dstField := newStruct.Field(i) + if dstField.CanSet() { + dstField.Set(deepCopyValue(srcField)) + } + }tests/core-providers/scenarios/validation_presets.go (1)
307-308: Don’t narrow Reasoning markers; restore full set (regression).Overriding ShouldContainAnyOf drops useful markers (“next”, “reasoning”, “think”, “analysis”, “conclusion”, “solution”). Keep the original broader list and include “solve”. This mirrors prior feedback.
- if requiresReasoning, ok := customParams["requires_reasoning"].(bool); ok && requiresReasoning { - expectations.ShouldContainAnyOf = []string{"step", "first", "then", "calculate", "therefore", "because", "solve"} - } + if requiresReasoning, ok := customParams["requires_reasoning"].(bool); ok && requiresReasoning { + expectations.ShouldContainAnyOf = []string{ + "step", "first", "then", "next", "calculate", "therefore", "because", + "reasoning", "think", "analysis", "conclusion", "solution", "solve", + } + }
🧹 Nitpick comments (19)
plugins/jsonparser/changelog.md (1)
4-4: Consider adding the plugin version and a more descriptive message.The changelog entry only mentions core and framework version updates without noting the jsonparser plugin's own version bump (1.3.15 per the AI summary) or what actually changed. A more descriptive entry would better communicate the scope of changes to users.
Consider revising to something like:
-- chore: version update core to 1.2.13 and framework to 1.1.15 +- chore: version bump to 1.3.15; core to 1.2.13, framework to 1.1.15Or, if there are specific changes to document (per the PR objectives: bug fixes, feature additions, test improvements):
-- chore: version update core to 1.2.13 and framework to 1.1.15 +- chore: version bump to 1.3.15; updated for core 1.2.13, framework 1.1.15 (bug fixes and test improvements).github/workflows/scripts/release-core.sh (1)
38-42: Consider using a subshell to simplify directory navigation.The new test step correctly integrates into the release flow after build validation. However, the directory navigation pattern could be improved for clarity and maintainability. Instead of multiple
cdcommands with relative paths, using a subshell pattern makes it explicit that you're returning to the original directory and reduces the risk of directory state issues.Apply this diff to improve clarity using a subshell:
-# Run core provider tests -echo "🔧 Running core provider tests..." -cd tests/core-providers -go test -v ./... -cd ../.. +# Run core provider tests +echo "🔧 Running core provider tests..." +(cd tests/core-providers && go test -v ./...)This pattern automatically restores the current directory after the subshell exits, making the intent clearer and reducing the cognitive load of tracking directory state across sequential
cdcommands.tests/core-providers/cross_provider_test.go (2)
13-14: Document why the test is skipped and remove redundant return.The test is unconditionally disabled, but there's no explanation for why or when it should be re-enabled. Consider:
- Adding a more descriptive skip message explaining the rationale (e.g., "flaky", "too expensive for CI", "needs refactoring")
- Adding a TODO comment with an issue reference to track re-enabling
- Removing the redundant
returnstatement—t.Skip()already halts executionExample:
func TestCrossProviderScenarios(t *testing.T) { - t.Skip("Skipping cross provider scenarios test") - return + // TODO(issue-XXX): Re-enable once cross-provider test stability is improved + t.Skip("Temporarily disabled: cross-provider scenarios are flaky in CI")
16-113: Consider removing or commenting out the unreachable code.All code after the skip becomes dead code and will never execute. If this is a temporary skip, consider adding a comment indicating that the code will be re-enabled. If permanent, the code should be removed to improve maintainability.
Options:
- If temporary: Add a comment above the skip explaining the code is preserved for re-enabling
- If long-term/permanent: Remove the dead code (can always restore from git history)
tests/core-providers/scenarios/multi_turn_conversation.go (1)
23-25: LGTM! Optional: Consider a helper function for this repo-wide pattern.The parallel test gating logic is correct and provides useful flexibility for CI/CD environments. Since the AI summary notes this is a "repo-wide pattern used across multiple test scenarios," you might want to extract this into a helper function to improve maintainability and reduce duplication.
Example helper:
// In a shared test utilities file func MaybeParallel(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() } }Then use it as:
t.Run("MultiTurnConversation", func(t *testing.T) { - if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { - t.Parallel() - } + MaybeParallel(t)tests/core-providers/scenarios/transcription.go (1)
241-243: Consider normalizing the environment variable check.While this specific location doesn't have a loop variable capture issue (not in a loop), the environment variable check pattern appears multiple times. Consider accepting common truthy values like "1" or case-insensitive "true"/"TRUE" for better developer experience.
For example:
- if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { + val := os.Getenv("SKIP_PARALLEL_TESTS") + if !strings.EqualFold(val, "true") && val != "1" { t.Parallel() }However, fixing the critical loop variable capture issues should be prioritized first.
Based on past review comments.
tests/core-providers/anthropic_test.go (1)
50-50: Consider deferring shutdown for panic safety.The explicit
client.Shutdown()call won't execute if the subtest panics. The previousdeferpattern provided better cleanup guarantees.Consider restoring the defer pattern:
+ defer client.Shutdown() + t.Run("AnthropicTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/cerebras_test.go (1)
49-49: Consider deferring shutdown for panic safety.The explicit
client.Shutdown()call won't execute if the subtest panics. Consider usingdefer client.Shutdown()before the subtest for better cleanup guarantees.tests/core-providers/gemini_test.go (1)
55-55: Consider deferring shutdown for panic safety.The explicit
client.Shutdown()call won't execute if the subtest panics. Consider usingdefer client.Shutdown()before the subtest for better cleanup guarantees.tests/core-providers/openrouter_test.go (1)
48-48: Consider deferring shutdown for panic safety.The explicit
client.Shutdown()call won't execute if the subtest panics. Consider usingdefer client.Shutdown()before the subtest for better cleanup guarantees.plugins/semanticcache/test_utils.go (1)
114-298: Consider regex pattern specificity.The mock rules are comprehensive, but some patterns could be improved:
- Line 148: Exact match
"What is the capital of France\\?"is very specific and won't match minor variations.- Line 252: Pattern
(?i)test.*message|test.*no-store|test.*cache|test.*error|ttl test|threshold test|provider.*test|edge case testis quite broad and might match unintended messages.These patterns work for current tests, but consider making them more flexible or more specific depending on your testing needs.
tests/core-providers/openai_test.go (1)
58-58: Consider deferring shutdown for panic safety.The explicit
client.Shutdown()call won't execute if the subtest panics. The previousdeferpattern provided better cleanup guarantees.Consider restoring the defer pattern:
+ defer client.Shutdown() + t.Run("OpenAITests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/scenarios/test_retry_framework.go (2)
261-270: Deprecated validation stub always passes; hook real validators.WithTestRetry logs deprecation and forces Passed=true, defeating retry-on-validation semantics. Replace with scenario-appropriate validators or remove this wrapper where unused.
Would you like a follow-up patch to route by request type (chat/responses/text) and call the specific Validate* functions?
941-945: Use StreamingRetryConfig for streaming transcription scenario.GetTestRetryConfigForScenario maps "TranscriptionStream" to DefaultTranscriptionRetryConfig, not streaming-specific conditions. Prefer StreamingRetryConfig for stream wrappers.
- case "Transcription", "TranscriptionStream": // 🎙️ Transcription tests - return DefaultTranscriptionRetryConfig() + case "Transcription": + return DefaultTranscriptionRetryConfig() + case "TranscriptionStream": + return StreamingRetryConfig()tests/core-providers/scenarios/transcription_stream.go (1)
333-347: Use StreamingRetryConfig in advanced stream subtests.The keys "TranscriptionStreamJSON/Lang/Prompt" aren’t handled and default to non-stream conditions. Use StreamingRetryConfig() for these wrappers.
- retryConfig := GetTestRetryConfigForScenario("TranscriptionStreamJSON", testConfig) + retryConfig := StreamingRetryConfig() ... - retryConfig := GetTestRetryConfigForScenario("TranscriptionStreamLang", testConfig) + retryConfig := StreamingRetryConfig() ... - retryConfig := GetTestRetryConfigForScenario("TranscriptionStreamPrompt", testConfig) + retryConfig := StreamingRetryConfig()Also applies to: 435-447, 523-537
tests/core-providers/scenarios/reasoning.go (1)
103-114: Minor: simplify empty-content log.This branch is unlikely if ShouldHaveContent is enforced; consider logging a concise summary always (first N chars).
tests/core-providers/scenarios/text_completion_stream.go (1)
451-452: Preserve base ExtraFields; merge instead of overwrite.You assign ExtraFields earlier, then overwrite with last chunk’s ExtraFields. Merge to avoid losing pre-set fields if last chunk omits them.
- consolidatedResponse.ExtraFields = lastResponse.BifrostTextCompletionResponse.ExtraFields + // Merge last chunk's extra fields over base (provider/request type preserved) + last := lastResponse.BifrostTextCompletionResponse.ExtraFields + base := consolidatedResponse.ExtraFields + if last.Provider != "" { base.Provider = last.Provider } + if last.RequestType != "" { base.RequestType = last.RequestType } + if last.ModelRequested != "" { base.ModelRequested = last.ModelRequested } + if last.Latency > 0 { base.Latency = last.Latency } + consolidatedResponse.ExtraFields = basetests/core-providers/scenarios/response_validation.go (1)
1045-1053: Align has_data metric with validation.You record has_data := response.Data != nil; use len(response.Data) > 0 to match validation.
- result.MetricsCollected["has_data"] = response.Data != nil + result.MetricsCollected["has_data"] = len(response.Data) > 0tests/core-providers/scenarios/validation_presets.go (1)
415-479: CombineExpectations cannot flip booleans from true→false.Current merge only overrides when new value is truthy; you can’t disable flags like ShouldHaveContent downstream. Consider tri-state or pointer booleans if you need explicit false overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (75)
.github/workflows/scripts/release-core.sh(1 hunks).gitignore(1 hunks)core/bifrost.go(1 hunks)core/changelog.md(1 hunks)core/providers/openai_test.go(1 hunks)core/providers/test_helpers.go(1 hunks)core/schemas/bifrost.go(1 hunks)core/version(1 hunks)docs/architecture/core/concurrency.mdx(1 hunks)docs/features/plugins/mocker.mdx(5 hunks)framework/changelog.md(1 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/main.go(6 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(0 hunks)plugins/semanticcache/plugin_responses_test.go(3 hunks)plugins/semanticcache/test_utils.go(4 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/anthropic_test.go(3 hunks)tests/core-providers/azure_test.go(3 hunks)tests/core-providers/bedrock_test.go(2 hunks)tests/core-providers/cerebras_test.go(3 hunks)tests/core-providers/cohere_test.go(3 hunks)tests/core-providers/config/account.go(17 hunks)tests/core-providers/cross_provider_test.go(1 hunks)tests/core-providers/custom_test.go(1 hunks)tests/core-providers/gemini_test.go(3 hunks)tests/core-providers/groq_test.go(3 hunks)tests/core-providers/mistral_test.go(3 hunks)tests/core-providers/ollama_test.go(3 hunks)tests/core-providers/openai_test.go(3 hunks)tests/core-providers/openrouter_test.go(2 hunks)tests/core-providers/parasail_test.go(3 hunks)tests/core-providers/scenarios/automatic_function_calling.go(4 hunks)tests/core-providers/scenarios/chat_completion_stream.go(8 hunks)tests/core-providers/scenarios/complete_end_to_end.go(2 hunks)tests/core-providers/scenarios/cross_provider_scenarios.go(1 hunks)tests/core-providers/scenarios/embedding.go(3 hunks)tests/core-providers/scenarios/end_to_end_tool_calling.go(2 hunks)tests/core-providers/scenarios/image_base64.go(3 hunks)tests/core-providers/scenarios/image_url.go(2 hunks)tests/core-providers/scenarios/multi_turn_conversation.go(2 hunks)tests/core-providers/scenarios/multiple_images.go(2 hunks)tests/core-providers/scenarios/multiple_tool_calls.go(4 hunks)tests/core-providers/scenarios/reasoning.go(4 hunks)tests/core-providers/scenarios/response_validation.go(10 hunks)tests/core-providers/scenarios/responses_stream.go(6 hunks)tests/core-providers/scenarios/simple_chat.go(3 hunks)tests/core-providers/scenarios/speech_synthesis.go(4 hunks)tests/core-providers/scenarios/speech_synthesis_stream.go(6 hunks)tests/core-providers/scenarios/test_retry_framework.go(4 hunks)tests/core-providers/scenarios/text_completion.go(2 hunks)tests/core-providers/scenarios/text_completion_stream.go(10 hunks)tests/core-providers/scenarios/tool_calls.go(3 hunks)tests/core-providers/scenarios/transcription.go(5 hunks)tests/core-providers/scenarios/transcription_stream.go(6 hunks)tests/core-providers/scenarios/validation_presets.go(7 hunks)tests/core-providers/sgl_test.go(3 hunks)tests/core-providers/vertex_test.go(3 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_edge_cases_test.go
✅ Files skipped from review due to trivial changes (3)
- plugins/telemetry/changelog.md
- plugins/governance/version
- plugins/logging/changelog.md
🚧 Files skipped from review as they are similar to previous changes (33)
- tests/core-providers/scenarios/multiple_images.go
- core/bifrost.go
- tests/core-providers/scenarios/text_completion.go
- plugins/mocker/changelog.md
- tests/core-providers/azure_test.go
- core/schemas/bifrost.go
- tests/core-providers/scenarios/end_to_end_tool_calling.go
- transports/version
- plugins/maxim/version
- tests/core-providers/scenarios/simple_chat.go
- core/version
- tests/core-providers/vertex_test.go
- framework/changelog.md
- tests/core-providers/cohere_test.go
- tests/core-providers/scenarios/automatic_function_calling.go
- plugins/maxim/changelog.md
- tests/core-providers/groq_test.go
- tests/core-providers/sgl_test.go
- framework/version
- docs/features/plugins/mocker.mdx
- transports/changelog.md
- tests/core-providers/scenarios/image_url.go
- plugins/otel/version
- tests/core-providers/scenarios/chat_completion_stream.go
- tests/core-providers/scenarios/responses_stream.go
- plugins/governance/changelog.md
- tests/core-providers/scenarios/image_base64.go
- plugins/semanticcache/version
- tests/core-providers/parasail_test.go
- plugins/telemetry/version
- plugins/jsonparser/version
- plugins/semanticcache/plugin_responses_test.go
- tests/core-providers/scenarios/complete_end_to_end.go
🧰 Additional context used
🧬 Code graph analysis (11)
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)
tests/core-providers/scenarios/response_validation.go (1)
plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/scenarios/speech_synthesis_stream.go (2)
core/schemas/speech.go (1)
BifrostSpeechStreamResponse(107-112)tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)
core/providers/test_helpers.go (2)
core/schemas/logger.go (3)
Logger(28-55)LogLevel(6-6)LoggerOutputType(17-17)core/schemas/bifrost.go (2)
BifrostResponse(213-223)BifrostError(319-328)
tests/core-providers/scenarios/text_completion_stream.go (2)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/textcompletions.go (1)
BifrostTextCompletionResponse(64-72)
plugins/semanticcache/test_utils.go (4)
plugins/mocker/main.go (7)
MockRule(77-85)Conditions(89-94)Response(98-104)ResponseTypeSuccess(28-28)SuccessResponse(108-115)MockerConfig(68-73)Init(156-202)plugins/semanticcache/main.go (2)
Plugin(137-144)Init(263-327)core/bifrost.go (2)
Bifrost(34-51)Init(80-194)core/schemas/bifrost.go (1)
BifrostConfig(21-29)
tests/core-providers/scenarios/transcription_stream.go (3)
core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(89-96)plugins/mocker/main.go (1)
Latency(126-130)tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)
plugins/mocker/main.go (3)
core/schemas/bifrost.go (6)
BifrostContextKey(96-96)RequestType(79-79)ChatCompletionRequest(84-84)ResponsesRequest(86-86)BifrostResponse(213-223)BifrostResponseExtraFields(251-259)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)ResponsesInputMessageRoleAssistant(304-304)ResponsesMessageTypeMessage(263-263)
tests/core-providers/config/account.go (4)
core/schemas/account.go (1)
AzureKeyConfig(21-25)ui/lib/types/config.ts (1)
AzureKeyConfig(20-24)core/utils.go (1)
Ptr(12-14)core/schemas/provider.go (1)
DefaultRequestTimeoutInSeconds(14-14)
tests/core-providers/scenarios/test_retry_framework.go (2)
core/schemas/bifrost.go (1)
BifrostStream(284-291)tests/core-providers/scenarios/test_retry_conditions.go (1)
EmptyResponseCondition(16-16)
tests/core-providers/bedrock_test.go (1)
core/schemas/bifrost.go (2)
Fallback(114-117)Bedrock(38-38)
🪛 LanguageTool
plugins/otel/changelog.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...his brings down inference overhead to < 1µseconds
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (18)
- 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
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (61)
.gitignore (2)
36-36: Good catch restoring the SQLite WAL ignore pattern.Re-adding
*.db-walensures SQLite Write-Ahead Log files (temporary runtime artifacts) remain untracked in version control.
38-38: Reasonable addition for local tooling artifacts.Adding
.claudeto ignore patterns prevents local Claude AI tool artifacts from being committed. This aligns with typical development environment cleanup practices.tests/core-providers/scenarios/cross_provider_scenarios.go (1)
700-700: LGTM! Trivial formatting adjustment.The comment indentation change has no functional impact.
core/changelog.md (1)
4-6: Changelog entries are clear and accurate.All three entries correctly document the PR changes with proper conventional commit formatting (bug/fix/feat) and clear scope references. The entries align with the PR objectives and use appropriate code reference syntax with backticks.
plugins/semanticcache/changelog.md (1)
4-5: Verify semanticcache plugin version bump and completeness of changelog entries.The PR summary mentions "Bumped versions across core, framework, and plugins," but this changelog only documents core and framework version updates. Two concerns:
- Was the semanticcache plugin itself versioned? If so, it should be documented in the changelog.
- The PR includes several changes (error logging fixes, embedding request handling, mocker enhancements, skip-mocker context key), but only the mocker test integration is captured here. Verify whether other semanticcache-specific changes warrant documentation.
tests/core-providers/scenarios/tool_calls.go (1)
6-6: LGTM! Flexible test parallelization control.The addition of the
osimport and the conditionalt.Parallel()execution based onSKIP_PARALLEL_TESTSprovides a useful mechanism to control test parallelization in different environments (e.g., CI/CD, resource-constrained systems). The logic correctly defaults to parallel execution when the environment variable is unset.Also applies to: 25-27
tests/core-providers/scenarios/embedding.go (2)
7-7: LGTM!The
osimport is correctly added to support the environment variable check for conditional parallelization.
52-54: LGTM!The conditional parallelization is a good addition, allowing tests to be controlled via environment variable when parallel execution causes issues.
tests/core-providers/scenarios/speech_synthesis.go (4)
61-63: LGTM: Parallelization guard correctly implemented.The conditional parallelization guard is correctly placed at the start of each test case subtest and follows the consistent pattern used across the test suite.
131-133: LGTM: Parallelization guard correctly applied.Consistent implementation of the parallelization guard for the advanced test scenario.
212-214: LGTM: Parallelization guard correctly applied to voice type subtests.The guard is properly placed within the loop to enable parallel execution of each voice type test case.
270-270: LGTM: Minor whitespace improvement.This empty line improves readability by visually separating the audio size validation from the model validation logic.
tests/core-providers/scenarios/multi_turn_conversation.go (1)
5-5: LGTM!The
osimport is correctly added to support the environment variable check on line 23.tests/core-providers/mistral_test.go (1)
13-15: LGTM: Environment guard prevents failures without credentials.The skip guard is well-placed and provides a clear message when credentials are unavailable.
tests/core-providers/ollama_test.go (1)
13-15: LGTM! Good practice to gate tests on required configuration.The environment guard prevents test execution when Ollama is not configured, which is consistent with the pattern used across other provider tests in this PR.
tests/core-providers/scenarios/speech_synthesis_stream.go (4)
6-6: LGTM: Import supports the parallelization guard.The
ospackage import is correctly added to support theSKIP_PARALLEL_TESTSenvironment variable checks throughout the test.
155-156: LGTM: Latency logging enhances observability.The per-chunk latency logging provides useful instrumentation for validating streaming performance, consistent with broader test enhancements.
178-178: LGTM: Deep copy prevents mutation issues.Using
DeepCopyBifrostStreamcorrectly captures an immutable snapshot of the streaming response, preventing potential mutation issues in concurrent test execution.
230-232: LGTM: Clean parallelization without capture issues.The conditional parallelization for
LongText_HDModel_Streamingis correctly implemented without loop variable capture concerns.tests/core-providers/anthropic_test.go (1)
13-15: Good test hygiene improvements.The environment-based skip guard and subtest wrapper improve test reliability and organization. This pattern is consistently applied across provider tests.
Also applies to: 47-49
tests/core-providers/cerebras_test.go (1)
13-15: Good test hygiene improvements.The environment-based skip guard and subtest wrapper improve test reliability and organization.
Also applies to: 46-48
tests/core-providers/gemini_test.go (2)
14-14: Good test hygiene improvements.The updated skip message and subtest wrapper improve test clarity and organization.
Also applies to: 52-54
42-42: Verify MultipleImages capability.The
MultipleImagesscenario was changed fromtruetofalse. Please ensure this accurately reflects Gemini's current capabilities.tests/core-providers/openrouter_test.go (2)
14-14: Good test hygiene improvements.The updated skip message and subtest wrapper improve test clarity and organization.
Also applies to: 45-47
36-41: Appropriate handling of beta features.Disabling tests for OpenRouter's beta Responses API is a reasonable approach until the API stabilizes.
plugins/semanticcache/test_utils.go (3)
14-14: LGTM.Mocker import is necessary for the test mocking infrastructure.
300-323: Review configuration consistency.
getMockedBifrostClientcreates a newBaseAccountinstance (line 312) instead of using configuration from the calling context. This means any custom account configuration passed toNewTestSetupWithConfigis ignored when creating the mocked client.If tests need custom account configurations, consider accepting account configuration as a parameter or reusing the config from the test setup.
356-376: Significant test behavior change - verify coverage.The switch from real API calls to mocked responses (line 376) changes test behavior fundamentally. While this improves test reliability and cost, ensure that integration tests with real APIs still exist elsewhere to catch provider-specific issues.
The comment on line 356 correctly notes that embeddings still use Weaviate, which is important for semantic cache functionality.
tests/core-providers/openai_test.go (1)
13-15: Good test hygiene improvements.The environment-based skip guard and subtest wrapper improve test reliability and organization. This pattern is consistently applied across all provider tests in this PR.
Also applies to: 55-57
core/providers/openai_test.go (5)
11-60: LGTM! Well-structured integration test.The test correctly skips when credentials are missing, sets up the provider with appropriate config, and validates the response structure.
62-129: LGTM! Function calling test is well-implemented.The test correctly defines tool schema with required parameters and validates that tool calls are returned.
131-181: LGTM! Streaming test properly validates chunks.The test correctly uses the mock post-hook runner and validates that streaming produces multiple chunks.
226-266: LGTM! Embedding test validates vector structure correctly.The test properly checks for non-nil response, data array, and embedding vector dimensions.
268-282: LGTM! Simple and effective provider key validation.plugins/mocker/main.go (3)
495-502: LGTM! Skip-mocker context key and request type filtering work correctly.The per-request bypass via context key is properly implemented with safe type assertion, and limiting to ChatCompletionRequest and ResponsesRequest types is appropriate for the mocker plugin's scope.
599-622: Excellent fix for model matching logic!Line 604 improves provider matching using
slices.Contains, and lines 613-618 fix a critical bug where model matching was comparing a string against a slice instead of iterating through individual models.
806-855: Response generation branches correctly by request type.The code properly creates ChatResponse for ChatCompletionRequest and ResponsesResponse for ResponsesRequest, with appropriate field mappings and usage conversions.
core/providers/test_helpers.go (1)
1-40: LGTM! Clean and focused test utilities.The no-op logger implementation, pointer helpers, and mock post-hook runner are all correctly implemented and appropriately scoped as private package functions. These utilities will reduce boilerplate in provider tests.
tests/core-providers/config/account.go (4)
134-134: Clarify the purpose of mixing chat and embedding models in one key configuration.The second Bedrock key lists both a chat model (
anthropic.claude-3-5-sonnet-20240620-v1:0) and an embedding model (cohere.embed-v4:0) in the same Models array, while the first key has an empty Models array but uses Deployments mapping.Ensure this configuration aligns with your key scoping strategy. Typically, keys either:
- Allow all models (empty Models array)
- Restrict to specific model types
- Use Deployments for friendly-name mapping
156-183: Well-structured Azure configuration refactor.The split into separate keys for chat (gpt-4o) and embeddings (text-embedding-ada-002) with dedicated endpoints is clean. The APIVersion field with environment override and helpful comments improves maintainability.
Minor suggestion: Consider validating that AZURE_EMB_API_KEY and AZURE_EMB_ENDPOINT are set when embedding tests run, similar to the AWS credentials check in bedrock_test.go.
257-257: Timeout increase to 120s improves test reliability.The consistent 4x increase (from default 30s to 120s) across all providers reduces test flakiness and accommodates slower models/regions. This aligns with the PR's focus on test improvements and latency calculations.
Also applies to: 271-271, 297-297, 310-310, 323-323, 336-336, 349-349, 362-362, 376-376, 389-389, 403-403, 416-416, 429-429, 442-442, 455-455
128-129: The Bedrock deployment mappings are valid and region-available. Both "global.anthropic.claude-sonnet-4-20250514-v1:0" (global cross-region) and "us.anthropic.claude-3-7-sonnet-20250219-v1:0" (US-region) are documented Bedrock inference profile IDs. No changes needed.tests/core-providers/bedrock_test.go (3)
39-44: Test scenarios correctly reflect Bedrock capabilities.The scenario flags accurately represent Bedrock's support:
- ImageURL and MultipleImages correctly disabled with explanatory comments
- Reasoning enabled to test the new ReasoningModel field
- ImageBase64 remains enabled (supported by Bedrock)
51-54: Good test structure improvements.Wrapping tests in
t.Run("BedrockTests", ...)enables better test organization, clearer output, and potential parallel execution. The explicitclient.Shutdown()is appropriate here since no cleanup is needed after the test run completes.
46-48: Verify fallback model name resolution.The fallback uses friendly name
"claude-3.7-sonnet"which should resolve via the Deployments mapping in config/account.go (line 129). Ensure your model resolver correctly handles this mapping for fallback scenarios.Run this script to verify fallback handling logic:
tests/core-providers/scenarios/transcription_stream.go (3)
63-66: Parallel gating LGTM.
Consistent SKIP_PARALLEL_TESTS guard.
215-216: Deep copy on lastResponse is correct.Prevents aliasing of pooled structs.
185-187: ****The latency unit is correct. ExtraFields.Latency is defined as int64 with the explicit comment "in milliseconds", and all providers consistently use
.Milliseconds()when populating this field (e.g.,time.Since(startTime).Milliseconds()). The test log correctly reports "ms".tests/core-providers/scenarios/reasoning.go (2)
28-30: Parallel gating LGTM.
Consistent with suite.
54-55: Fallbacks propagation LGTM.
Matches other scenarios.tests/core-providers/scenarios/multiple_tool_calls.go (3)
31-33: Parallel gating LGTM.
85-86: Fallbacks propagation LGTM across both APIs.Also applies to: 98-99
92-99: Review comment is based on non-existent field.No
ResponsesModelfield exists inComprehensiveTestConfigor anywhere in the codebase. The code correctly usestestConfig.ChatModelfor the Responses request, which is consistent with the Chat operation above it (line 82). The suggested refactor cannot be implemented.Likely an incorrect or invalid review comment.
tests/core-providers/scenarios/text_completion_stream.go (3)
24-26: Parallel gating LGTM.
100-100: Deep copy lastResponse LGTM.
Prevents pooled-struct mutation issues.
111-113: Latency logging and assertion LGTM.
Tightens telemetry and validation.Also applies to: 181-188
tests/core-providers/scenarios/response_validation.go (4)
41-42: Latency expectation added — good.Enables uniform telemetry checks.
493-502: Latency validation across response types LGTM.Captures latency_ms in metrics; fails when missing.
Also applies to: 666-675, 843-851, 909-917, 974-982, 1028-1035
1002-1006: Embedding nil/empty handling LGTM.Switch to len(data)==0 avoids nil-only checks.
1288-1289: Switch to t.Errorf on failure — good.Keeps detailed errors while failing test.
tests/core-providers/scenarios/validation_presets.go (1)
25-26: Global latency requirement LGTM.
Brings consistency across scenarios.Also applies to: 114-115, 165-165, 183-184, 213-213
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
bff252b to
9f9801b
Compare
Pratham-Mishra04
force-pushed
the
10-22-feat_enhacements_token_details_for_chat_completions_and_added_latency_calculation_in_vertex
branch
from
fbdd429 to
2f89e37
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
9f9801b to
3c90f16
Compare
Pratham-Mishra04
force-pushed
the
10-22-feat_enhacements_token_details_for_chat_completions_and_added_latency_calculation_in_vertex
branch
from
2f89e37 to
badb1db
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
3c90f16 to
691f3c0
Compare
Pratham-Mishra04
force-pushed
the
10-22-feat_enhacements_token_details_for_chat_completions_and_added_latency_calculation_in_vertex
branch
from
badb1db to
8fd0545
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
3 times, most recently
from
0d05428 to
83263a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
core/providers/vertex.go (1)
297-319: Error parsing may panic; fix nil deref and shadowing; tighten fallbacks.If unmarshalling into openAIErr succeeds but Error is nil, accessing openAIErr.Error.Message will panic. There’s also variable shadowing on vertexErr. Consolidate logic and guard nil.
Apply:
@@ - var openAIErr schemas.BifrostError - - var vertexErr []VertexError - if err := sonic.Unmarshal(body, &openAIErr); err != nil { - // Try Vertex error format if OpenAI format fails - if err := sonic.Unmarshal(body, &vertexErr); err != nil { - - //try with single Vertex error format - var vertexErr VertexError - if err := sonic.Unmarshal(body, &vertexErr); err != nil { - return nil, newBifrostOperationError(schemas.ErrProviderResponseUnmarshal, err, schemas.Vertex) - } - - return nil, newProviderAPIError(vertexErr.Error.Message, nil, resp.StatusCode, schemas.Vertex, nil, nil) - } - - if len(vertexErr) > 0 { - return nil, newProviderAPIError(vertexErr[0].Error.Message, nil, resp.StatusCode, schemas.Vertex, nil, nil) - } - } - - return nil, newProviderAPIError(openAIErr.Error.Message, nil, resp.StatusCode, schemas.Vertex, nil, nil) + var openAIErr schemas.BifrostError + if err := sonic.Unmarshal(body, &openAIErr); err == nil && openAIErr.Error != nil && openAIErr.Error.Message != "" { + return nil, newProviderAPIError(openAIErr.Error.Message, nil, resp.StatusCode, schemas.Vertex, nil, nil) + } + // Try Vertex single-error format + var vErr VertexError + if err := sonic.Unmarshal(body, &vErr); err == nil && vErr.Error.Message != "" { + return nil, newProviderAPIError(vErr.Error.Message, nil, resp.StatusCode, schemas.Vertex, nil, nil) + } + // Try Vertex array format (seen via proxies) + var vErrList []VertexError + if err := sonic.Unmarshal(body, &vErrList); err == nil && len(vErrList) > 0 && vErrList[0].Error.Message != "" { + return nil, newProviderAPIError(vErrList[0].Error.Message, nil, resp.StatusCode, schemas.Vertex, nil, nil) + } + // Unknown/unsupported schema + return nil, newBifrostOperationError(schemas.ErrProviderResponseUnmarshal, fmt.Errorf("status=%d body=%s", resp.StatusCode, string(body)), schemas.Vertex)core/schemas/providers/gemini/utils.go (1)
355-464: Fix nil-deref on message.Content and map roles to Gemini’s expected values.
Content can be nil (tool/assistant/tool-call messages). Also, Gemini accepts role “user” or “model”; passing “assistant”/“tool”/“system” can be invalid. Guard Content access and map roles; omit otherwise.Apply this patch:
@@ -func convertBifrostMessagesToGemini(messages []schemas.ChatMessage) []Content { - var contents []Content +func convertBifrostMessagesToGemini(messages []schemas.ChatMessage) []Content { + var contents []Content for _, message := range messages { - var parts []*Part + var parts []*Part @@ - // Handle content - if message.Content.ContentStr != nil && *message.Content.ContentStr != "" { - parts = append(parts, &Part{ - Text: *message.Content.ContentStr, - }) - } else if message.Content.ContentBlocks != nil { + // Handle content (guard nil) + if message.Content != nil && message.Content.ContentStr != nil && *message.Content.ContentStr != "" { + parts = append(parts, &Part{Text: *message.Content.ContentStr}) + } else if message.Content != nil && message.Content.ContentBlocks != nil { for _, block := range message.Content.ContentBlocks { if block.Text != nil { - parts = append(parts, &Part{ - Text: *block.Text, - }) + parts = append(parts, &Part{Text: *block.Text}) } } } @@ - // Handle tool response messages + // Handle tool response messages if message.Role == schemas.ChatMessageRoleTool && message.ChatToolMessage != nil { @@ - // Extract content string from ContentStr or ContentBlocks - if message.Content.ContentStr != nil && *message.Content.ContentStr != "" { + // Extract content string from ContentStr or ContentBlocks (guard nil) + if message.Content != nil && message.Content.ContentStr != nil && *message.Content.ContentStr != "" { contentStr = *message.Content.ContentStr - } else if message.Content.ContentBlocks != nil { + } else if message.Content != nil && message.Content.ContentBlocks != nil { @@ - if len(parts) > 0 { - content := Content{ - Parts: parts, - Role: string(message.Role), - } - contents = append(contents, content) - } + if len(parts) > 0 { + // Map Bifrost roles to Gemini roles; omit others to let SDK infer + roleStr := "" + switch message.Role { + case schemas.ChatMessageRoleUser: + roleStr = "user" + case schemas.ChatMessageRoleAssistant: + roleStr = "model" + } + content := Content{Parts: parts} + if roleStr != "" { + content.Role = roleStr + } + contents = append(contents, content) + } } return contents }plugins/mocker/main.go (4)
515-518: Probability=0 does not disable the rule (logic bug).
Current check applies the rule when Probability==0. Treat 0 as “never”.Apply this diff:
- // Check if rule should activate based on probability (0.0 = never, 1.0 = always) - if rule.Probability > 0 && rand.Float64() > rule.Probability { + // Check if rule should activate based on probability (0.0 = never, 1.0 = always) + if rule.Probability == 0 { + // Disabled via probability + return req, nil, nil + } + if rand.Float64() > rule.Probability { // Rule didn't activate due to probability, continue with normal flow return req, nil, nil }
858-861: Nil‑deref when overriding model for ResponsesRequest.
mockResponse.ChatResponse is nil for ResponsesRequest; deref panics.Apply this diff:
- if content.Model != nil { - mockResponse.ChatResponse.Model = *content.Model - } + if content.Model != nil { + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.Model = *content.Model + mockResponse.ChatResponse.ExtraFields.ModelRequested = *content.Model + } + if mockResponse.ResponsesResponse != nil { + mockResponse.ResponsesResponse.ExtraFields.ModelRequested = *content.Model + } + }
872-875: RawResponse is written to a copy and discarded.
GetExtraFields likely returns by value; assign directly to underlying response.Apply this diff:
- extraFields := mockResponse.GetExtraFields() - extraFields.RawResponse = rawResponse + if mockResponse.ChatResponse != nil { + mockResponse.ChatResponse.ExtraFields.RawResponse = rawResponse + } else if mockResponse.ResponsesResponse != nil { + mockResponse.ResponsesResponse.ExtraFields.RawResponse = rawResponse + }
520-525: Populate ExtraFields.Latency on mocked successes.
You simulate delay but don’t record it; new validators expect latency>0.Apply this diff:
- // Apply artificial latency simulation if configured - if latency := p.getLatency(&rule.MockRule); latency != nil { - delay := p.calculateLatency(latency) - time.Sleep(delay) - } + // Apply artificial latency simulation if configured and capture applied latency in ms + var appliedLatencyMs int64 = 0 + if latency := p.getLatency(&rule.MockRule); latency != nil { + delay := p.calculateLatency(latency) + time.Sleep(delay) + appliedLatencyMs = int64(delay / time.Millisecond) + } else { + // Ensure a non-zero value to satisfy validators even without explicit latency config + appliedLatencyMs = 1 + } @@ - if response.Type == ResponseTypeSuccess { - return p.generateSuccessShortCircuit(req, response) + if response.Type == ResponseTypeSuccess { + scReq, sc, err := p.generateSuccessShortCircuit(req, response) + if sc != nil && sc.Response != nil { + if sc.Response.ChatResponse != nil { + sc.Response.ChatResponse.ExtraFields.Latency = appliedLatencyMs + } else if sc.Response.ResponsesResponse != nil { + sc.Response.ResponsesResponse.ExtraFields.Latency = appliedLatencyMs + } + } + return scReq, sc, errAlso applies to: 541-546
♻️ Duplicate comments (16)
docs/architecture/core/concurrency.mdx (1)
271-286: Restore the pool recursion edge (duplicate issue from prior review).This is a re-occurrence of a previously flagged issue. The diagram still lacks the "ReturnObject --> GetObject" edge that completes the object pool lifecycle. Without it, the diagram doesn't show that returned objects are available for reuse in subsequent
GetObjectrequests, which is the core benefit of object pooling. The newPoolChecknode improves the flow logic, but the cycle must be preserved.Apply this diff to restore the pool recursion:
GetObject --> PoolCheck PoolCheck -- Yes --> NewObject PoolCheck -- No --> UseObject NewObject --> UseObject UseObject --> ResetObject ResetObject --> ReturnObject + ReturnObject --> GetObject endplugins/otel/changelog.md (1)
5-6: Fix grammar and unit formatting in changelog entries.Two minor text quality issues remain unaddressed from previous reviews:
- Line 5: "Value prefixed" should be "Values prefixed" (or "Configuration values prefixed") for grammatical correctness.
- Line 6: "< 1µseconds" should be formatted as "< 1 µs" or "< 1 microsecond" for proper unit notation.
Apply this diff:
-- feat: added headers support for OTel configuration. Value prefixed with env will be fetched from environment variables (env.<ENV_VAR_NAME>) -- feat: emission of OTel resource spans is completely async - this brings down inference overhead to < 1µseconds +- feat: added headers support for OTel configuration. Values prefixed with env will be fetched from environment variables (env.<ENV_VAR_NAME>) +- feat: emission of OTel resource spans is completely async - this brings down inference overhead to < 1 µstests/core-providers/vertex_test.go (1)
13-15: Skip guard still allows tests to run without required project IDThe current logic only requires
VERTEX_PROJECT_IDwhenVERTEX_API_KEYis empty. If someone setsVERTEX_API_KEYbut forgetsVERTEX_PROJECT_ID, the guard doesn't skip, and the test will fail later inconfig.SetupTest(). Vertex requires a project ID in all scenarios.Apply this diff to always require the project ID:
- if os.Getenv("VERTEX_API_KEY") == "" && (os.Getenv("VERTEX_PROJECT_ID") == "" || os.Getenv("VERTEX_CREDENTIALS") == "") { - t.Skip("Skipping Vertex tests because VERTEX_API_KEY is not set and VERTEX_PROJECT_ID or VERTEX_CREDENTIALS is not set") - } + if os.Getenv("VERTEX_PROJECT_ID") == "" || (os.Getenv("VERTEX_API_KEY") == "" && os.Getenv("VERTEX_CREDENTIALS") == "") { + t.Skip("Skipping Vertex tests; set VERTEX_PROJECT_ID and either VERTEX_API_KEY or VERTEX_CREDENTIALS") + }tests/core-providers/ollama_test.go (1)
45-48: Verify: explicit shutdown after subtest won't execute on panic or fatal error.The explicit
client.Shutdown()on line 48 has the same resource-leak risk previously flagged: ifrunAllComprehensiveTestspanics or triggers a fatal test failure, the shutdown won't execute. The previous review suggested usingdefer client.Shutdown()(marked as addressed), but the current code still uses an explicit post-subtest call.Since the AI summary indicates this explicit pattern is now common across all provider tests, please verify:
- Was there a specific reason the defer pattern wasn't adopted?
- Does the test framework or infrastructure guarantee cleanup despite explicit calls?
tests/core-providers/scenarios/speech_synthesis_stream.go (5)
367-446: Loop variable capture still incorrect with t.Parallel().This issue was previously raised but not properly fixed. The
voiceloop variable is captured at line 373 (voiceCopy := voice) AFTERt.Parallel()is called (lines 369-371), andvoiceis used directly in multiple locations (lines 391, 398, 408, 416, 421, 431, 436, 440). When parallel subtests unblock and execute these lines, the loop may have already progressed, causing all subtests to see the last value ofvoice, leading to incorrect test names, logs, and retry contexts.Apply this diff to properly capture the loop variable:
for _, voice := range voices { + voice := voice // capture loop var before t.Run t.Run("StreamingVoice_"+voice, func(t *testing.T) { if os.Getenv("SKIP_PARALLEL_TESTS") != "true" { t.Parallel() } - voiceCopy := voice + voiceCopy := voice // now safe to copy request := &schemas.BifrostSpeechRequest{ Provider: testConfig.Provider, Model: testConfig.SpeechSynthesisModel,
286-289: Same context bypass issue as in main test.Same issue as line 117-120: a new
context.Background()is created instead of using the passedctxparameter.Apply the same fix as suggested earlier to use the passed context.
342-344: Same latency validation issue as in main test.Same issue as line 217-219: the test fails if
lastTokenLatency == 0, which may produce false positives.
402-405: Same context bypass issue as in main test.Same issue as lines 117-120 and 286-289: a new
context.Background()is created instead of using the passedctxparameter.
442-444: Same latency validation issue as in main test.Same issue as lines 217-219 and 342-344: the test fails if
lastTokenLatency == 0, which may produce false positives.tests/core-providers/scenarios/validation_presets.go (1)
303-308: Narrowing the reasoning keyword list reduces test coverage.Line 306 replaces the comprehensive ShouldContainAnyOf list from ReasoningExpectations() (which includes "next", "reasoning", "think", "analysis", "conclusion", "solution") with a shorter set. This makes tests more brittle and can miss valid reasoning responses that use the dropped keywords.
Consider preserving the full default set or merging additional keywords instead of replacing.
Apply this diff to preserve the full keyword set:
case "Reasoning": expectations := ReasoningExpectations() - if requiresReasoning, ok := customParams["requires_reasoning"].(bool); ok && requiresReasoning { - expectations.ShouldContainAnyOf = []string{"step", "first", "then", "calculate", "therefore", "because", "solve"} - } + // The default ReasoningExpectations already includes a comprehensive set of reasoning keywords + // No need to override when requires_reasoning is true return expectationstests/core-providers/scenarios/test_retry_framework.go (1)
52-57: Critical: Deep copy checks source field settability instead of destination.Checking
field.CanSet()on the source field at line 54 is incorrect. The check must be on the destination field's settability to actually perform the copy. This bug can result in incomplete or zeroed copies.Apply this fix:
for i := 0; i < original.NumField(); i++ { - field := original.Field(i) - if field.CanSet() { - newStruct.Field(i).Set(deepCopyValue(field)) - } + srcField := original.Field(i) + dstField := newStruct.Field(i) + if dstField.CanSet() { + dstField.Set(deepCopyValue(srcField)) + } }tests/core-providers/cohere_test.go (1)
38-40: The previous review comment about misleading comments remains unaddressed.The inline comments on lines 38-40 still reference "c4ai-aya-vision-8b model" while the test uses "command-a-vision-07-2025" (line 26). This inconsistency was flagged in a previous review and has not been corrected.
tests/core-providers/azure_test.go (1)
13-15: Guard embedding scenarios when AZURE_EMB_API_KEY is absent.This test enables embeddings (line 46) but only checks
AZURE_API_KEY. WithoutAZURE_EMB_API_KEY, embedding calls may fail or use incorrect credentials.Add a check after testConfig initialization:
} + + // Disable embedding if embeddings key is not provided + if os.Getenv("AZURE_EMB_API_KEY") == "" { + t.Log("AZURE_EMB_API_KEY not set; disabling Azure embedding scenarios") + testConfig.EmbeddingModel = "" + testConfig.Scenarios.Embedding = false + } t.Run("AzureTests", func(t *testing.T) {tests/core-providers/custom_test.go (1)
44-47: Guarantee cleanup with t.Cleanup for panic-safety.Use t.Cleanup(func(){ client.Shutdown() }) so teardown runs even if the subtest panics; then drop the explicit call after t.Run.
- t.Run("CustomProviderTests", func(t *testing.T) { + t.Cleanup(func() { client.Shutdown() }) + t.Run("CustomProviderTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()tests/core-providers/scenarios/embedding.go (1)
104-112: Fix compile error and 2D indexing bug in embedding extraction.
- for i := range responseDataLength does not compile; must use an indexed loop.
- When responseDataLength derives from 2D arrays, indexing response.Data[i] can go out of bounds. Also nil-check Embedding before deref.
Apply this diff:
@@ - embeddings := make([][]float32, len(testTexts)) - responseDataLength := len(response.Data) - if responseDataLength != len(testTexts) { - if responseDataLength > 0 && response.Data[0].Embedding.Embedding2DArray != nil { - responseDataLength = len(response.Data[0].Embedding.Embedding2DArray) - } - if responseDataLength != len(testTexts) { - t.Fatalf("Expected %d embedding results, got %d", len(testTexts), responseDataLength) - } - } - - for i := range responseDataLength { - vec, extractErr := getEmbeddingVector(response.Data[i]) + embeddings := make([][]float32, len(testTexts)) + responseDataLength := len(response.Data) + sourceIs2D := false + if responseDataLength != len(testTexts) { + if responseDataLength > 0 { + emb := response.Data[0].Embedding + if emb != nil && emb.Embedding2DArray != nil { + sourceIs2D = true + responseDataLength = len(emb.Embedding2DArray) + } + } + if responseDataLength != len(testTexts) { + t.Fatalf("Expected %d embedding results, got %d", len(testTexts), responseDataLength) + } + } + + for i := 0; i < responseDataLength; i++ { + var ( + vec []float32 + extractErr error + ) + if sourceIs2D { + vec, extractErr = getEmbeddingVectorFrom2D(response.Data[0], i) + } else { + if i >= len(response.Data) { + t.Fatalf("Index %d out of range for response.Data (len=%d)", i, len(response.Data)) + } + vec, extractErr = getEmbeddingVector(response.Data[i]) + } if extractErr != nil { t.Fatalf("Failed to extract embedding vector for text '%s': %v", testTexts[i], extractErr) } @@ }Add this helper in the same file:
+// getEmbeddingVectorFrom2D extracts the i-th vector from a 2D embeddings payload. +func getEmbeddingVectorFrom2D(d schemas.EmbeddingData, i int) ([]float32, error) { + if d.Embedding == nil || d.Embedding.Embedding2DArray == nil { + return nil, fmt.Errorf("2D embeddings not present") + } + rows := d.Embedding.Embedding2DArray // [][]float32 + if i < 0 || i >= len(rows) { + return nil, fmt.Errorf("index %d out of range (len=%d)", i, len(rows)) + } + return rows[i], nil +}Also applies to: 114-114
tests/core-providers/config/account.go (1)
370-370: BaseURL configuration remains unresolved.As noted in a previous review, setting
BaseURLdirectly fromos.Getenv("OLLAMA_BASE_URL")without a default will result in an empty string if the environment variable is unset, causing Ollama requests to fail. Consider applying the previously suggested fix or ensuring tests skip when the environment variable is not set.
🧹 Nitpick comments (17)
.github/workflows/scripts/release-core.sh (1)
38-42: Add defensive checks for directory navigation.The script relies on
set -eto catchcdfailures, which is safe. However, explicitly validating the test directory exists and providing clearer error messages would improve robustness and debuggability.Consider adding an explicit check for the test directory before navigation:
# Run core provider tests echo "🔧 Running core provider tests..." + +if [[ ! -d tests/core-providers ]]; then + echo "❌ tests/core-providers directory not found" + exit 1 +fi + cd tests/core-providers go test -v ./... cd ../..This provides a clearer error message if the directory is missing and makes the dependency explicit.
tests/core-providers/cross_provider_test.go (2)
13-15: Remove redundant return and consider cleaning up unreachable code.The
returnstatement aftert.Skip()is unnecessary sincet.Skip()terminates test execution by callingruntime.Goexit(). Additionally, approximately 100 lines of setup and test code (lines 16-112) are now unreachable.Consider:
- Remove the redundant
returnstatement- If permanently skipping, remove the dead code to improve maintainability
- If temporarily skipping, add a TODO comment or issue reference explaining when/why these tests should be re-enabled
- Enhance the skip message with context, e.g.,
"Skipping cross provider scenarios - see issue #XXX"or"Skipping - requires external provider credentials"Apply this diff if permanently skipping:
func TestCrossProviderScenarios(t *testing.T) { t.Skip("Skipping cross provider scenarios test") - return - - client, ctx, cancel, err := config.SetupTest() - if err != nil { - t.Fatalf("Error initializing test setup: %v", err) - } - defer cancel() - defer client.Shutdown() - - // Define available providers for cross-provider testing - providers := []scenarios.ProviderConfig{ - // ... (remove all unreachable code) - } - // ... (remove remaining test logic) }Or if temporarily skipping, add context:
func TestCrossProviderScenarios(t *testing.T) { - t.Skip("Skipping cross provider scenarios test") - return + // TODO(#XXX): Re-enable once testing infrastructure is updated + t.Skip("Skipping cross provider scenarios - pending infrastructure changes")
116-118: Remove redundant return and consider cleaning up unreachable code.Same issue as
TestCrossProviderScenarios: thereturnstatement aftert.Skip()is redundant, and approximately 30 lines of unreachable code remain (lines 119-147).Apply the same refactoring approach as suggested for the first test function.
Apply this diff if permanently skipping:
func TestCrossProviderConsistency(t *testing.T) { t.Skip("Skipping cross provider consistency test") - return - - client, ctx, cancel, err := config.SetupTest() - // ... (remove all unreachable code) }tests/core-providers/vertex_test.go (1)
46-49: Consider restoring defer for shutdown to ensure cleanup on panicMoving from
defer client.Shutdown()to an explicit call after the subtest means shutdown won't execute ifrunAllComprehensiveTestspanics. Usingdeferensures cleanup regardless of how the test exits, and matches the pattern already used forcancel()on line 21.Apply this diff to restore the defer pattern:
- t.Run("VertexTests", func(t *testing.T) { - runAllComprehensiveTests(t, client, ctx, testConfig) - }) - client.Shutdown() + defer client.Shutdown() + t.Run("VertexTests", func(t *testing.T) { + runAllComprehensiveTests(t, client, ctx, testConfig) + })core/providers/groq.go (1)
129-137: Consider debug logging when conversion returns nil.When
ToTextCompletionResponse()returns nil, the response is silently dropped. While this may be intentional for empty chunks or metadata in streaming responses, adding debug-level logging would improve observability and help diagnose unexpected conversion failures.Example:
if textCompletionResponse != nil { textCompletionResponse.ExtraFields.RequestType = schemas.TextCompletionRequest textCompletionResponse.ExtraFields.Provider = provider.GetProviderKey() textCompletionResponse.ExtraFields.ModelRequested = request.Model responseChan <- &schemas.BifrostStream{ BifrostTextCompletionResponse: textCompletionResponse, } +} else { + provider.logger.Debug(ctx, "skipped chat response chunk: conversion to text completion returned nil") }core/providers/gemini.go (3)
384-385: Consider memory implications of 10MB buffer per stream.The 10MB maximum buffer size is reasonable for handling large audio chunks, but be aware that each concurrent streaming connection will consume up to 10MB of buffer space. At scale (e.g., 100 concurrent streams = 1GB in buffers), this could impact memory usage significantly.
401-406: Preferstrings.CutPrefixfor consistency and efficiency.TranscriptionStream (lines 680-681) uses the more efficient
strings.CutPrefixpattern. Consider applying the same approach here for consistency.Apply this diff:
- var jsonData string - // Parse SSE data - if strings.HasPrefix(line, "data: ") { - jsonData = strings.TrimPrefix(line, "data: ") - } else { - // Handle raw JSON errors (without "data: " prefix) - jsonData = line - } + var jsonData string + // Parse SSE data + if after, ok := strings.CutPrefix(line, "data: "); ok { + jsonData = after + } else { + // Handle raw JSON errors (without "data: " prefix) + jsonData = line + }
692-724: Consider usingprocessGeminiStreamChunkhelper for consistency.SpeechStream uses the
processGeminiStreamChunkhelper function (line 414) to process streaming chunks, but TranscriptionStream manually parses the response here. Using the helper would improve consistency and reduce code duplication.You could refactor this section to:
- // First, check if this is an error response - var errorCheck map[string]interface{} - if err := sonic.Unmarshal([]byte(jsonData), &errorCheck); err != nil { - provider.logger.Warn(fmt.Sprintf("Failed to parse stream data as JSON: %v", err)) - continue - } - - // Handle error responses - if _, hasError := errorCheck["error"]; hasError { - bifrostErr := &schemas.BifrostError{ - Type: schemas.Ptr("gemini_api_error"), - IsBifrostError: false, - Error: &schemas.ErrorField{ - Message: fmt.Sprintf("Gemini API error: %v", errorCheck["error"]), - Error: fmt.Errorf("stream error: %v", errorCheck["error"]), - }, - ExtraFields: schemas.BifrostErrorExtraFields{ - RequestType: schemas.TranscriptionStreamRequest, - Provider: providerName, - ModelRequested: request.Model, - }, - } - ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) - processAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, provider.logger) - return - } - - // Parse Gemini streaming response - var geminiResponse gemini.GenerateContentResponse - if err := sonic.Unmarshal([]byte(jsonData), &geminiResponse); err != nil { - provider.logger.Warn(fmt.Sprintf("Failed to parse Gemini stream response: %v", err)) - continue - } + // Process chunk using shared function + geminiResponse, err := processGeminiStreamChunk(jsonData) + if err != nil { + if strings.Contains(err.Error(), "gemini api error") { + bifrostErr := &schemas.BifrostError{ + Type: schemas.Ptr("gemini_api_error"), + IsBifrostError: false, + Error: &schemas.ErrorField{ + Message: err.Error(), + Error: err, + }, + ExtraFields: schemas.BifrostErrorExtraFields{ + RequestType: schemas.TranscriptionStreamRequest, + Provider: providerName, + ModelRequested: request.Model, + }, + } + ctx = context.WithValue(ctx, schemas.BifrostContextKeyStreamEndIndicator, true) + processAndSendBifrostError(ctx, postHookRunner, bifrostErr, responseChan, provider.logger) + return + } + provider.logger.Warn(fmt.Sprintf("Failed to process chunk: %v", err)) + continue + }core/providers/vertex.go (1)
206-219: Deduplicate Vertex URL assembly for better maintainability.URL construction is duplicated across non-streaming and streaming paths. The endpoints themselves are correctly implemented per Vertex AI documentation (Anthropic rawPredict/streamRawPredict under v1, OpenAI-compatible under v1beta1, with proper global location support). However, consolidating the URL building into a single helper would improve maintainability and reduce drift risk.
Apply refactoring within these ranges:
@@ - var url string - if strings.Contains(request.Model, "claude") { - if region == "global" { - url = fmt.Sprintf("https://aiplatform.googleapis.com/v1/projects/%s/locations/global/publishers/anthropic/models/%s:rawPredict", projectID, request.Model) - } else { - url = fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/publishers/anthropic/models/%s:rawPredict", region, projectID, region, request.Model) - } - } else { - if region == "global" { - url = fmt.Sprintf("https://aiplatform.googleapis.com/v1beta1/projects/%s/locations/global/endpoints/openapi/chat/completions", projectID) - } else { - url = fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1beta1/projects/%s/locations/%s/endpoints/openapi/chat/completions", region, projectID, region) - } - } + url, err := buildVertexChatURL(projectID, region, request.Model, false) + if err != nil { + return nil, newBifrostOperationError("invalid vertex endpoint", err, schemas.Vertex) + }@@ - var url string - if region == "global" { - url = fmt.Sprintf("https://aiplatform.googleapis.com/v1/projects/%s/locations/global/publishers/anthropic/models/%s:streamRawPredict", projectID, request.Model) - } else { - url = fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/publishers/anthropic/models/%s:streamRawPredict", region, projectID, region, request.Model) - } + url, err := buildVertexChatURL(projectID, region, request.Model, true) + if err != nil { + return nil, newBifrostOperationError("invalid vertex endpoint", err, schemas.Vertex) + }@@ - var url string - if region == "global" { - url = fmt.Sprintf("https://aiplatform.googleapis.com/v1beta1/projects/%s/locations/global/endpoints/openapi/chat/completions", projectID) - } else { - url = fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1beta1/projects/%s/locations/%s/endpoints/openapi/chat/completions", region, projectID, region) - } + url, err := buildVertexChatURL(projectID, region, request.Model, true) + if err != nil { + return nil, newBifrostOperationError("invalid vertex endpoint", err, schemas.Vertex) + }Add helper (outside the changed ranges):
// buildVertexChatURL returns the correct Vertex endpoint for chat/stream requests. func buildVertexChatURL(projectID, region, model string, stream bool) (string, error) { if strings.Contains(model, "claude") { if region == "global" { if stream { return fmt.Sprintf("https://aiplatform.googleapis.com/v1/projects/%s/locations/global/publishers/anthropic/models/%s:streamRawPredict", projectID, model), nil } return fmt.Sprintf("https://aiplatform.googleapis.com/v1/projects/%s/locations/global/publishers/anthropic/models/%s:rawPredict", projectID, model), nil } if stream { return fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/publishers/anthropic/models/%s:streamRawPredict", region, projectID, region, model), nil } return fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1/projects/%s/locations/%s/publishers/anthropic/models/%s:rawPredict", region, projectID, region, model), nil } // OpenAI-compatible (same URL for stream/non-stream) if region == "global" { return fmt.Sprintf("https://aiplatform.googleapis.com/v1beta1/projects/%s/locations/global/endpoints/openapi/chat/completions", projectID), nil } return fmt.Sprintf("https://%s-aiplatform.googleapis.com/v1beta1/projects/%s/locations/%s/endpoints/openapi/chat/completions", region, projectID, region), nil }Also applies to: 414-419, 442-447
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
172-176: Consider case-insensitive keyword matching for robustness.The keyword change from "san francisco" to "francisco" works around case sensitivity by matching both "San Francisco" and "san francisco". However, this could potentially match false positives in other contexts.
If the test framework supports it, consider implementing case-insensitive keyword matching instead:
expectations2.ShouldContainKeywordsIgnoreCase = []string{"san francisco", "22", "sunny"}This would be more explicit and robust than substring matching. If not supported, the current approach is acceptable for this specific test scenario.
tests/core-providers/scenarios/text_completion_stream.go (3)
111-113: Optional: reduce log verbosity for large streams.Per-chunk latency logs can be noisy; consider gating behind an env var (e.g., DEBUG_STREAM_LOGS) or sample every N chunks.
181-188: Unify latency assertion semantics with Responses stream tests.Here missing latency triggers t.Errorf (test failure), while responses_stream.go treats it as a soft validation warning. Pick one policy and apply consistently, or gate strictness by provider capability.
Would you like this to be strictly enforced only for providers known to populate latency (e.g., Vertex), and downgraded to a warning otherwise?
451-452: Preserve RequestType/Provider when propagating ExtraFields.Overwriting ExtraFields loses the explicit RequestType/Provider set earlier. Merge and enforce expected values.
- consolidatedResponse.ExtraFields = lastResponse.BifrostTextCompletionResponse.ExtraFields + ex := lastResponse.BifrostTextCompletionResponse.ExtraFields + ex.Provider = provider + ex.RequestType = schemas.TextCompletionRequest + consolidatedResponse.ExtraFields = extests/core-providers/scenarios/responses_stream.go (2)
113-115: Optional: gate per‑chunk latency logs.Large streams become noisy; consider gating behind an env var or sampling.
611-618: Align latency validation policy across stream tests.This is non-fatal here but fatal in text_completion_stream.go. Recommend a consistent policy (strict for providers that populate latency; warning otherwise).
Would you prefer making latency required only when testConfig.Provider is known to emit it (e.g., via a capability flag)?
tests/core-providers/groq_test.go (1)
55-58: Prefer t.Cleanup for shutdown consistency.Use t.Cleanup so client.Shutdown always runs, then drop the explicit call.
- t.Run("GroqTests", func(t *testing.T) { + t.Cleanup(func() { client.Shutdown() }) + t.Run("GroqTests", func(t *testing.T) { runAllComprehensiveTests(t, client, ctx, testConfig) }) - client.Shutdown()core/schemas/providers/gemini/utils.go (1)
480-556: Optional: normalize MP3 MIME consistently.
detectAudioMimeType returns "audio/mp3"; normalizeAudioMIMEType maps "mp3" to "audio/mpeg". Consider standardizing on "audio/mpeg" to avoid provider-specific surprises.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (89)
.github/workflows/scripts/release-core.sh(1 hunks).gitignore(1 hunks)core/bifrost.go(3 hunks)core/changelog.md(1 hunks)core/providers/gemini.go(3 hunks)core/providers/groq.go(1 hunks)core/providers/test_helpers.go(1 hunks)core/providers/vertex.go(4 hunks)core/schemas/bifrost.go(1 hunks)core/schemas/providers/gemini/chat.go(1 hunks)core/schemas/providers/gemini/embedding.go(1 hunks)core/schemas/providers/gemini/responses.go(9 hunks)core/schemas/providers/gemini/speech.go(1 hunks)core/schemas/providers/gemini/transcription.go(1 hunks)core/schemas/providers/gemini/types.go(2 hunks)core/schemas/providers/gemini/utils.go(4 hunks)core/schemas/providers/openai/chat.go(1 hunks)core/schemas/providers/openai/types.go(1 hunks)core/version(1 hunks)docs/architecture/core/concurrency.mdx(1 hunks)docs/features/plugins/mocker.mdx(5 hunks)framework/changelog.md(1 hunks)framework/version(1 hunks)plugins/governance/changelog.md(1 hunks)plugins/governance/version(1 hunks)plugins/jsonparser/changelog.md(1 hunks)plugins/jsonparser/version(1 hunks)plugins/logging/changelog.md(1 hunks)plugins/logging/version(1 hunks)plugins/maxim/changelog.md(1 hunks)plugins/maxim/version(1 hunks)plugins/mocker/changelog.md(1 hunks)plugins/mocker/main.go(6 hunks)plugins/mocker/version(1 hunks)plugins/otel/changelog.md(1 hunks)plugins/otel/version(1 hunks)plugins/semanticcache/changelog.md(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(0 hunks)plugins/semanticcache/plugin_responses_test.go(3 hunks)plugins/semanticcache/test_utils.go(4 hunks)plugins/semanticcache/version(1 hunks)plugins/telemetry/changelog.md(1 hunks)plugins/telemetry/version(1 hunks)tests/core-providers/anthropic_test.go(3 hunks)tests/core-providers/azure_test.go(3 hunks)tests/core-providers/bedrock_test.go(2 hunks)tests/core-providers/cerebras_test.go(3 hunks)tests/core-providers/cohere_test.go(3 hunks)tests/core-providers/config/account.go(8 hunks)tests/core-providers/cross_provider_test.go(2 hunks)tests/core-providers/custom_test.go(2 hunks)tests/core-providers/gemini_test.go(2 hunks)tests/core-providers/groq_test.go(3 hunks)tests/core-providers/mistral_test.go(3 hunks)tests/core-providers/ollama_test.go(3 hunks)tests/core-providers/openai_test.go(3 hunks)tests/core-providers/openrouter_test.go(2 hunks)tests/core-providers/parasail_test.go(3 hunks)tests/core-providers/scenarios/automatic_function_calling.go(4 hunks)tests/core-providers/scenarios/chat_completion_stream.go(8 hunks)tests/core-providers/scenarios/complete_end_to_end.go(10 hunks)tests/core-providers/scenarios/cross_provider_scenarios.go(1 hunks)tests/core-providers/scenarios/embedding.go(4 hunks)tests/core-providers/scenarios/end_to_end_tool_calling.go(3 hunks)tests/core-providers/scenarios/image_base64.go(3 hunks)tests/core-providers/scenarios/image_url.go(2 hunks)tests/core-providers/scenarios/multi_turn_conversation.go(2 hunks)tests/core-providers/scenarios/multiple_images.go(2 hunks)tests/core-providers/scenarios/multiple_tool_calls.go(4 hunks)tests/core-providers/scenarios/reasoning.go(4 hunks)tests/core-providers/scenarios/response_validation.go(10 hunks)tests/core-providers/scenarios/responses_stream.go(6 hunks)tests/core-providers/scenarios/simple_chat.go(4 hunks)tests/core-providers/scenarios/speech_synthesis.go(9 hunks)tests/core-providers/scenarios/speech_synthesis_stream.go(16 hunks)tests/core-providers/scenarios/test_retry_framework.go(4 hunks)tests/core-providers/scenarios/text_completion.go(2 hunks)tests/core-providers/scenarios/text_completion_stream.go(13 hunks)tests/core-providers/scenarios/tool_calls.go(3 hunks)tests/core-providers/scenarios/transcription.go(11 hunks)tests/core-providers/scenarios/transcription_stream.go(15 hunks)tests/core-providers/scenarios/utils.go(1 hunks)tests/core-providers/scenarios/validation_presets.go(8 hunks)tests/core-providers/sgl_test.go(3 hunks)tests/core-providers/vertex_test.go(3 hunks)transports/bifrost-http/handlers/middlewares.go(1 hunks)transports/changelog.md(1 hunks)transports/version(1 hunks)ui/app/providers/views/modelProviderKeysTableView.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/semanticcache/plugin_edge_cases_test.go
✅ Files skipped from review due to trivial changes (6)
- plugins/jsonparser/version
- plugins/telemetry/changelog.md
- plugins/jsonparser/changelog.md
- plugins/otel/version
- .gitignore
- transports/changelog.md
🚧 Files skipped from review as they are similar to previous changes (26)
- core/bifrost.go
- tests/core-providers/scenarios/image_url.go
- docs/features/plugins/mocker.mdx
- tests/core-providers/scenarios/reasoning.go
- plugins/semanticcache/changelog.md
- tests/core-providers/scenarios/transcription.go
- tests/core-providers/bedrock_test.go
- tests/core-providers/mistral_test.go
- transports/version
- core/schemas/bifrost.go
- plugins/mocker/version
- plugins/mocker/changelog.md
- plugins/maxim/version
- plugins/semanticcache/version
- core/changelog.md
- plugins/telemetry/version
- plugins/governance/changelog.md
- tests/core-providers/scenarios/speech_synthesis.go
- core/version
- plugins/semanticcache/plugin_responses_test.go
- plugins/governance/version
- tests/core-providers/scenarios/cross_provider_scenarios.go
- plugins/logging/version
- tests/core-providers/parasail_test.go
- framework/changelog.md
- plugins/logging/changelog.md
🧰 Additional context used
🧬 Code graph analysis (30)
tests/core-providers/gemini_test.go (3)
tests/core-providers/config/setup.go (1)
SetupTest(51-60)tests/core-providers/config/account.go (1)
ComprehensiveTestConfig(43-59)core/schemas/bifrost.go (2)
Gemini(47-47)Fallback(114-117)
core/schemas/providers/gemini/embedding.go (1)
core/schemas/providers/gemini/types.go (2)
Content(868-876)Part(882-906)
tests/core-providers/scenarios/end_to_end_tool_calling.go (1)
tests/core-providers/scenarios/validation_presets.go (2)
ConversationExpectations(131-137)ModifyExpectationsForProvider(326-373)
tests/core-providers/openai_test.go (3)
tests/core-providers/config/setup.go (1)
SetupTest(51-60)tests/core-providers/config/account.go (1)
ComprehensiveTestConfig(43-59)core/schemas/bifrost.go (2)
OpenAI(35-35)Fallback(114-117)
tests/core-providers/anthropic_test.go (3)
tests/core-providers/config/setup.go (1)
SetupTest(51-60)tests/core-providers/config/account.go (1)
ComprehensiveTestConfig(43-59)core/schemas/bifrost.go (2)
Anthropic(37-37)Fallback(114-117)
tests/core-providers/scenarios/chat_completion_stream.go (4)
core/schemas/chatcompletions.go (3)
ChatMessage(336-345)BifrostChatResponse(20-30)ChatStreamResponseChoice(518-520)tests/core-providers/scenarios/utils.go (1)
CreateBasicChatMessage(218-225)tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)tests/core-providers/scenarios/validation_presets.go (2)
GetExpectationsForScenario(230-319)ModifyExpectationsForProvider(326-373)
tests/core-providers/scenarios/responses_stream.go (3)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/responses.go (4)
BifrostResponsesStreamResponse(1334-1367)ResponsesMessage(291-303)ResponsesInputMessageRoleUser(309-309)ResponsesMessageContent(315-318)plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/scenarios/complete_end_to_end.go (6)
tests/core-providers/scenarios/test_retry_framework.go (4)
GetTestRetryConfigForScenario(919-949)TestRetryContext(139-144)WithDualAPITestRetry(807-916)TestRetryConfig(147-154)tests/core-providers/scenarios/validation_presets.go (3)
ConversationExpectations(131-137)ModifyExpectationsForProvider(326-373)VisionExpectations(140-154)core/schemas/chatcompletions.go (3)
BifrostChatResponse(20-30)BifrostChatRequest(11-17)ChatParameters(144-173)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)BifrostResponsesRequest(32-38)ResponsesParameters(74-101)tests/core-providers/scenarios/utils.go (3)
GetErrorMessage(550-574)GetChatContent(300-327)GetResponsesContent(346-391)tests/core-providers/scenarios/response_validation.go (1)
ResponseExpectations(18-45)
core/schemas/providers/gemini/speech.go (1)
core/schemas/providers/gemini/types.go (2)
Content(868-876)Part(882-906)
tests/core-providers/azure_test.go (3)
tests/core-providers/config/setup.go (1)
SetupTest(51-60)tests/core-providers/config/account.go (1)
ComprehensiveTestConfig(43-59)core/schemas/bifrost.go (2)
Azure(36-36)Fallback(114-117)
tests/core-providers/scenarios/response_validation.go (1)
plugins/mocker/main.go (1)
Latency(126-130)
tests/core-providers/cerebras_test.go (3)
tests/core-providers/config/setup.go (1)
SetupTest(51-60)tests/core-providers/config/account.go (1)
ComprehensiveTestConfig(43-59)core/schemas/bifrost.go (2)
Cerebras(46-46)Fallback(114-117)
tests/core-providers/scenarios/text_completion_stream.go (2)
tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)core/schemas/textcompletions.go (1)
BifrostTextCompletionResponse(64-72)
core/schemas/providers/gemini/utils.go (2)
core/schemas/chatcompletions.go (1)
ChatMessage(336-345)core/schemas/providers/gemini/types.go (2)
Content(868-876)Part(882-906)
plugins/semanticcache/test_utils.go (5)
plugins/mocker/main.go (7)
MockRule(77-85)Conditions(89-94)Response(98-104)ResponseTypeSuccess(28-28)SuccessResponse(108-115)MockerConfig(68-73)Init(156-202)plugins/semanticcache/main.go (2)
Plugin(137-144)Init(263-327)core/bifrost.go (2)
Bifrost(34-51)Init(80-194)core/schemas/bifrost.go (1)
BifrostConfig(21-29)core/schemas/account.go (1)
Account(53-69)
ui/app/providers/views/modelProviderKeysTableView.tsx (1)
ui/components/ui/alertDialog.tsx (1)
AlertDialogCancel(85-85)
tests/core-providers/scenarios/speech_synthesis_stream.go (4)
core/schemas/provider.go (1)
Provider(194-219)core/schemas/bifrost.go (2)
BifrostStream(284-291)BifrostError(319-328)tests/core-providers/scenarios/test_retry_framework.go (2)
WithStreamRetry(541-583)DeepCopyBifrostStream(18-25)core/schemas/speech.go (1)
BifrostSpeechStreamResponse(107-112)
core/providers/test_helpers.go (2)
core/schemas/logger.go (3)
Logger(28-55)LogLevel(6-6)LoggerOutputType(17-17)core/schemas/bifrost.go (2)
BifrostResponse(213-223)BifrostError(319-328)
core/schemas/providers/gemini/transcription.go (1)
core/schemas/providers/gemini/types.go (3)
Part(882-906)Blob(909-917)Content(868-876)
tests/core-providers/groq_test.go (3)
tests/core-providers/config/setup.go (1)
SetupTest(51-60)tests/core-providers/config/account.go (1)
ComprehensiveTestConfig(43-59)core/schemas/bifrost.go (3)
Groq(43-43)Fallback(114-117)BifrostContextKey(96-96)
core/schemas/providers/openai/chat.go (2)
core/schemas/bifrost.go (1)
Mistral(41-41)core/schemas/chatcompletions.go (1)
ChatToolChoiceStruct(248-253)
core/schemas/providers/gemini/responses.go (2)
core/schemas/responses.go (5)
ResponsesMessage(291-303)ResponsesInputMessageRoleSystem(310-310)ResponsesMessageContentBlock(373-384)ResponsesInputMessageContentBlockTypeText(362-362)ResponsesInputMessageContentBlockFile(391-395)core/schemas/providers/gemini/types.go (6)
Content(868-876)Role(9-9)Part(882-906)Type(769-769)Blob(909-917)FileData(967-975)
tests/core-providers/scenarios/text_completion.go (1)
core/schemas/textcompletions.go (2)
BifrostTextCompletionRequest(10-16)TextCompletionInput(74-77)
plugins/mocker/main.go (3)
core/schemas/bifrost.go (6)
BifrostContextKey(96-96)RequestType(79-79)ChatCompletionRequest(84-84)ResponsesRequest(86-86)BifrostResponse(213-223)BifrostResponseExtraFields(251-259)core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/responses.go (3)
BifrostResponsesResponse(40-72)ResponsesInputMessageRoleAssistant(308-308)ResponsesMessageTypeMessage(267-267)
core/providers/groq.go (4)
core/schemas/chatcompletions.go (1)
BifrostChatResponse(20-30)core/schemas/bifrost.go (3)
RequestType(79-79)TextCompletionRequest(82-82)BifrostStream(284-291)core/schemas/provider.go (1)
Provider(194-219)core/schemas/textcompletions.go (1)
BifrostTextCompletionResponse(64-72)
core/providers/vertex.go (2)
core/schemas/provider.go (1)
ErrProviderResponseUnmarshal(25-25)core/schemas/bifrost.go (1)
Vertex(40-40)
tests/core-providers/scenarios/transcription_stream.go (5)
tests/core-providers/scenarios/utils.go (4)
TTSTestTextBasic(17-17)TTSTestTextMedium(20-20)TTSTestTextTechnical(23-23)GenerateTTSAudioForTest(503-548)core/utils.go (1)
Ptr(12-14)core/schemas/bifrost.go (2)
BifrostStream(284-291)BifrostError(319-328)core/schemas/transcriptions.go (1)
BifrostTranscriptionStreamResponse(89-96)tests/core-providers/scenarios/test_retry_framework.go (1)
DeepCopyBifrostStream(18-25)
tests/core-providers/config/account.go (5)
core/schemas/provider.go (5)
Provider(194-219)DefaultRequestTimeoutInSeconds(14-14)ConcurrencyAndBufferSize(53-56)ProviderConfig(148-156)NetworkConfig(34-42)core/schemas/bifrost.go (13)
ModelProvider(32-32)Fallback(114-117)Bedrock(38-38)Cohere(39-39)Azure(36-36)Vertex(40-40)Ollama(42-42)Mistral(41-41)SGL(44-44)Parasail(45-45)Cerebras(46-46)Gemini(47-47)OpenRouter(48-48)ui/lib/types/config.ts (4)
ModelProvider(158-160)AzureKeyConfig(20-24)ConcurrencyAndBufferSize(97-100)NetworkConfig(87-94)core/schemas/account.go (1)
AzureKeyConfig(21-25)core/utils.go (1)
Ptr(12-14)
tests/core-providers/scenarios/test_retry_framework.go (2)
core/schemas/bifrost.go (1)
BifrostStream(284-291)tests/core-providers/scenarios/test_retry_conditions.go (1)
EmptyResponseCondition(16-16)
tests/core-providers/custom_test.go (3)
core/schemas/provider.go (1)
Provider(194-219)tests/core-providers/config/account.go (1)
ProviderOpenAICustom(17-17)core/schemas/bifrost.go (1)
Fallback(114-117)
🪛 LanguageTool
plugins/otel/changelog.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...his brings down inference overhead to < 1µseconds
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ 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). (13)
- 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
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
83263a8 to
fc1b40e
Compare
Pratham-Mishra04
force-pushed
the
10-22-chore_tests_fixes
branch
from
fc1b40e to
9063e88
Compare
Merge activity
|
akshaydeo
changed the base branch from
10-22-feat_enhacements_token_details_for_chat_completions_and_added_latency_calculation_in_vertex
to
graphite-base/661
Summary
This PR fixes two bugs: improves error logging for primary provider failures and adds support for embedding requests in the
GetExtraFields()method ofBifrostResponse. It also enhances the mocker plugin with responses request support and adds a context key to skip the mocker plugin per request.Changes
handleRequestto properly display error messages from primary providersGetExtraFields()method ofBifrostResponseType of change
Affected areas
How to test
Test the improved error logging:
Breaking changes
Related issues
Fixes embedding request handling in
GetExtraFields()methodSecurity considerations
No security implications.
Checklist