refactor: standardize error handling across providers by Pratham-Mishra04 · Pull Request #179 · maximhq/bifrost

Pratham-Mishra04 · 2025-07-22T09:57:05Z

Standardize Error Handling Across Providers

This PR introduces a consistent error handling approach across all LLM providers by implementing standardized error creation functions. The changes:

Add three new helper functions for creating consistent error types:
- newConfigurationError: For errors related to missing or invalid configuration
- newBifrostOperationError: For internal Bifrost operation errors
- newProviderAPIError: For errors returned by provider APIs
Rename utility functions to follow Go naming conventions:
- ProcessAndSendResponse → processAndSendResponse
- Add new processAndSendError function to handle stream errors consistently
Refactor error handling in all providers (OpenAI, Anthropic, Azure, Bedrock, Cohere, Groq, Mistral, Ollama, Vertex) to use the new standardized error functions
Improve error handling in streaming implementations to properly process errors through post-hook runners
Extract common form data handling logic in OpenAI's transcription methods to reduce code duplication

These changes make the codebase more maintainable by reducing duplication, ensuring consistent error structures, and improving error propagation through the system.

coderabbitai · 2025-07-22T09:57:10Z

Summary by CodeRabbit

Refactor
- Standardized error handling across all providers for improved consistency and maintainability.
- Centralized error creation using new helper functions, reducing code duplication.
- Enhanced streaming error handling and response processing for more reliable error propagation.
- Minor internal function renaming and modularization for better code organization.

No changes to user-facing features or interfaces.

Walkthrough

This update refactors error handling across all provider implementations by introducing centralized helper functions for constructing error responses. It replaces inline BifrostError struct creation with standardized error constructors and updates internal streaming response handling to use new utility functions. Some function names are adjusted for consistency, and duplicated code is modularized.

Changes

Files/Group	Change Summary
core/providers/utils.go	Adds centralized error helper functions; renames `ProcessAndSendResponse` to `processAndSendResponse`; adds `processAndSendError`.
core/providers/anthropic.go, core/providers/bedrock.go, core/providers/cohere.go, core/providers/openai.go	Refactor error handling to use new helper functions; update streaming error and response handling; rename response send function.
core/providers/azure.go, core/providers/groq.go	Replace inline error struct construction with helper function calls for error creation.
core/providers/mistral.go, core/providers/ollama.go	Refactor error handling to use new error helpers for marshaling and config errors.
core/providers/vertex.go	Standardize error handling using new helpers for configuration, operation, and API errors.

Sequence Diagram(s)

sequenceDiagram
    participant Provider
    participant Utils
    participant PostHookRunner
    participant ResponseChan

    Provider->>Utils: newBifrostOperationError / newConfigurationError / newProviderAPIError
    Provider->>Utils: processAndSendResponse(ctx, postHookRunner, response, responseChan)
    Utils->>PostHookRunner: Run post-hook on response/error
    PostHookRunner-->>Utils: Post-processed response/error
    Utils->>ResponseChan: Send response/error (if context not cancelled)

Estimated code review effort

4 (~80 minutes)

Possibly related PRs

feat: add streaming support for chat completions #162: Refactors and enhances streaming chat completion error handling and response processing in core/providers/anthropic.go, directly related to this PR’s centralization of error handling and streaming logic.
feat: azure and vertex config to keys and optimised key management in store #170: Updates Anthropic provider key handling and method signatures, related by provider and method overlap.
feat: Add Speech and Transcription API Support #177: Adds speech and transcription API support including streaming, related due to overlapping provider streaming implementations.

Suggested reviewers

danpiths
akshaydeo

Poem

In the warren of code, the errors would hop,
But now they're corralled in a centralized stop.
Helpers abound for each error type,
While streaming flows smoothly, all bugs out of sight.
With helpers and hooks, the review will be light!
🐇✨

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch 07-22-enhancement_error_handling_improvements_in_core_providers

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai generate unit tests to generate unit tests for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

Pratham-Mishra04 · 2025-07-22T09:57:24Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

coderabbitai

Actionable comments posted: 1

🔭 Outside diff range comments (4)

core/providers/vertex.go (2)
183-193: Inconsistent error handling pattern

This error handling still uses inline BifrostError construction, which is inconsistent with the refactoring pattern applied elsewhere in the file.

Apply this diff to maintain consistency:
-		return nil, &schemas.BifrostError{
-			IsBifrostError: false,
-			Error: schemas.ErrorField{
-				Message: schemas.ErrProviderRequest,
-				Error:   err,
-			},
-		}
+		return nil, newBifrostOperationError(schemas.ErrProviderRequest, err, schemas.Vertex)
209-219: Missing standardization for context cancellation errors

The context cancellation error handling still uses inline construction instead of the standardized helper functions.

Consider creating a specific helper for context-related errors or using newBifrostOperationError:
-			return nil, &schemas.BifrostError{
-				IsBifrostError: false,
-				Error: schemas.ErrorField{
-					Type:    StrPtr(schemas.RequestCancelled),
-					Message: fmt.Sprintf("Request cancelled or timed out by context: %v", ctx.Err()),
-					Error:   err,
-				},
-			}
+			return nil, newProviderAPIError(
+				fmt.Sprintf("Request cancelled or timed out by context: %v", ctx.Err()),
+				err,
+				0, // No HTTP status code for context errors
+				schemas.Vertex,
+				StrPtr(schemas.RequestCancelled),
+				nil,
+			)
core/providers/cohere.go (2)
198-217: Inconsistent error handling in ChatCompletion method

The error handling for request preparation and JSON marshaling in the ChatCompletion method still uses inline BifrostError construction, which is inconsistent with the standardization applied elsewhere.

Apply this diff for consistency:
-		return nil, &schemas.BifrostError{
-			IsBifrostError: true,
-			Error: schemas.ErrorField{
-				Message: "failed to prepare Cohere chat request",
-				Error:   err,
-			},
-		}
+		return nil, newBifrostOperationError("failed to prepare Cohere chat request", err, schemas.Cohere)

...

-		return nil, &schemas.BifrostError{
-			IsBifrostError: true,
-			Error: schemas.ErrorField{
-				Message: schemas.ErrProviderJSONMarshaling,
-				Error:   err,
-			},
-		}
+		return nil, newBifrostOperationError(schemas.ErrProviderJSONMarshaling, err, schemas.Cohere)
735-742: Missing error standardization for HTTP request errors

The HTTP request error handling in streaming still uses inline construction.

Apply this diff for consistency:
-		return nil, &schemas.BifrostError{
-			IsBifrostError: false,
-			Error: schemas.ErrorField{
-				Message: schemas.ErrProviderRequest,
-				Error:   err,
-			},
-		}
+		return nil, newBifrostOperationError(schemas.ErrProviderRequest, err, schemas.Cohere)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7998504 and ad63c52.

📒 Files selected for processing (10)

core/providers/anthropic.go (12 hunks)
core/providers/azure.go (9 hunks)
core/providers/bedrock.go (19 hunks)
core/providers/cohere.go (11 hunks)
core/providers/groq.go (1 hunks)
core/providers/mistral.go (5 hunks)
core/providers/ollama.go (1 hunks)
core/providers/openai.go (23 hunks)
core/providers/utils.go (3 hunks)
core/providers/vertex.go (6 hunks)

🧠 Learnings (11)

📓 Common learnings

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#162
File: transports/bifrost-http/integrations/genai/types.go:0-0
Timestamp: 2025-07-16T07:13:29.496Z
Learning: Pratham-Mishra04 prefers to avoid redundant error handling across architectural layers in the Bifrost streaming implementation. When error handling (such as timeouts, context cancellation, and JSON marshaling failures) is already handled at the provider level, they prefer not to duplicate this logic at the transport integration layer to keep the code simple and avoid unnecessary complexity.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#158
File: .github/workflows/transports-release.yml:30-43
Timestamp: 2025-07-18T11:12:28.861Z
Learning: Pratham-Mishra04 relies on branch protection rules and mandatory code reviews for security in the bifrost repository, preferring process controls over technical security measures like environment variable isolation for GitHub Actions workflows. All commits are reviewed before merging to main branch.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/types.go:89-119
Timestamp: 2025-06-10T11:00:02.875Z
Learning: In the Bifrost OpenAI integration (transports/bifrost-http/integrations/openai/types.go), the convertOpenAIContent function currently only handles the last image URL when multiple images are present in a content array. The user Pratham-Mishra04 has acknowledged this limitation and indicated it's part of a larger architectural issue that will be addressed comprehensively later, rather than with piecemeal fixes.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#150
File: transports/bifrost-http/lib/store.go:370-466
Timestamp: 2025-07-09T04:58:08.229Z
Learning: Pratham-Mishra04 prefers not to add logging or error handling for unreachable code paths in the Bifrost project. When provider types or similar entities are predefined in the system, defensive programming like logging in default cases is considered unnecessary overhead.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#84
File: transports/bifrost-http/main.go:2-2
Timestamp: 2025-06-15T16:05:13.489Z
Learning: For the Bifrost project, HTTP transport integration routers for new providers (like Mistral and Ollama) are implemented in separate PRs from the core provider support, following a focused PR strategy.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#94
File: core/bifrost.go:823-845
Timestamp: 2025-06-18T15:15:51.323Z
Learning: In the Bifrost project, the team prioritizes maintaining consistent error handling patterns over exposing detailed error context. All errors should be wrapped in the standard `BifrostError` structure rather than creating specific error types or exposing richer error details like exit codes or stderr output.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#152
File: transports/bifrost-http/plugins/logging/utils.go:378-399
Timestamp: 2025-07-10T13:44:14.518Z
Learning: In the Bifrost logging plugin (transports/bifrost-http/plugins/logging/utils.go), Pratham-Mishra04 prefers not to add error handling for JSON unmarshaling operations, considering logging not very critical and being confident that JSON marshalling won't fail in practice.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#141
File: core/bifrost.go:198-272
Timestamp: 2025-07-08T18:30:08.258Z
Learning: Pratham-Mishra04 follows a pattern of implementing core functionality first and deferring non-critical improvements (like race condition fixes, optimizations) to later PRs. This is a reasonable development approach that prioritizes getting the main feature working before addressing edge cases.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:96-101
Timestamp: 2025-06-15T13:50:41.418Z
Learning: In the Bifrost project, when a provider doesn't support a specific operation (like text completion), the IsBifrostError flag should be set to false to mark it as a provider-level error rather than a Bifrost framework error. This is intentional design for unsupported operations.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#152
File: transports/bifrost-http/plugins/logging/utils.go:94-111
Timestamp: 2025-07-10T13:44:23.297Z
Learning: Pratham-Mishra04 prefers not to add error handling for JSON marshaling operations in the Bifrost logging plugin (transports/bifrost-http/plugins/logging/utils.go) because logging is not critical functionality and the structured schema data being marshaled is unlikely to fail. They accept the risk of not handling json.Marshal errors in logging contexts to keep the code simple.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#83
File: core/providers/mistral.go:168-170
Timestamp: 2025-06-15T14:24:49.882Z
Learning: In the Bifrost codebase, performance is prioritized over defensive copying for HTTP service operations. Specifically, shallow slice assignments in provider response handling are acceptable due to object pool reset patterns and JSON unmarshaling behavior that minimize practical data corruption risks.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#63
File: transports/bifrost-http/integrations/openai/router.go:0-0
Timestamp: 2025-06-09T14:03:34.227Z
Learning: In the Bifrost HTTP transport layer (transports/bifrost-http/integrations/), request validation like checking for empty messages should be handled by the provider rather than at the transport layer. The transport layer should forward requests to Bifrost core/providers for validation.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/simple_chat.go:39-41
Timestamp: 2025-06-16T04:13:42.755Z
Learning: In the Bifrost codebase, errors returned from methods like ChatCompletionRequest are of type BifrostError (a custom error type) rather than the standard Go error interface. Therefore, require.Nilf should be used for error assertions instead of require.NoErrorf.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#81
File: tests/core-providers/scenarios/complete_end_to_end.go:39-41
Timestamp: 2025-06-16T04:12:05.427Z
Learning: In the Bifrost system, error returns are of type `BifrostError` rather than the standard Go `error` interface. Therefore, use `require.Nilf(t, err, ...)` instead of `require.NoError(t, err)` when checking for errors in Bifrost function calls.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#80
File: plugins/maxim/main.go:249-264
Timestamp: 2025-06-14T06:17:54.426Z
Learning: In the BifrostError struct, the Error field is a struct (not a pointer), so accessing bifrostErr.Error.Message, bifrostErr.Error.Code, and bifrostErr.Error.Type is safe without nil checks on the Error field itself. The Code and Type fields are of type *string.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#55
File: core/providers/anthropic.go:358-388
Timestamp: 2025-06-04T05:37:59.699Z
Learning: User Pratham-Mishra04 prefers not to extract small code duplications (around 2 lines) into helper functions, considering the overhead not worth it for such minor repetition.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#102
File: README.md:62-66
Timestamp: 2025-06-19T17:03:03.639Z
Learning: Pratham-Mishra04 prefers using the implicit 'latest' tag for the maximhq/bifrost Docker image rather than pinning to specific versions.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#143
File: core/mcp.go:155-196
Timestamp: 2025-07-08T15:33:47.698Z
Learning: Pratham-Mishra04 prefers not to add explanatory comments for obvious code patterns, such as the unlock/lock strategy around network I/O operations, considering them self-explanatory to experienced developers.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#138
File: docs/usage/go-package/mcp.md:408-412
Timestamp: 2025-07-01T12:40:08.576Z
Learning: Pratham-Mishra04 is okay with keeping bullet list formatting that uses colons after dashes in markdown documentation, even if it triggers linter warnings, preferring functionality over strict formatting rules.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#162
File: tests/core-providers/scenarios/chat_completion_stream.go:103-105
Timestamp: 2025-07-16T04:26:09.288Z
Learning: Pratham-Mishra04 prefers to keep test code simple when it serves its basic functional purpose. For tests that are meant to validate core functionality (like verifying streaming works), they consider hard-coded reasonable limits acceptable rather than making them configurable.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#148
File: transports/bifrost-http/lib/store.go:880-910
Timestamp: 2025-07-08T17:14:21.544Z
Learning: Pratham-Mishra04 prefers resilient system design where missing environment variables for MCP connections should not cause complete system failure. The system should continue processing other MCP connections even when some fail, maintaining partial functionality rather than implementing fail-fast behavior.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#138
File: transports/README.md:26-28
Timestamp: 2025-07-01T12:45:06.906Z
Learning: Pratham-Mishra04 prefers keeping documentation examples simple and concise, trusting users to handle production-specific considerations like version pinning themselves rather than cluttering examples with additional notes.

Learnt from: Pratham-Mishra04
PR: maximhq/bifrost#148
File: transports/bifrost-http/lib/store.go:1081-1098
Timestamp: 2025-07-08T17:16:50.811Z
Learning: Pratham-Mishra04 prefers practical redaction approaches over theoretical security improvements when the threat model is low-risk, such as admin-only interfaces in the Bifrost project. Fixed-length redaction is acceptable when only trusted administrators will see the redacted values.

core/providers/groq.go (10)

Learnt from: Pratham-Mishra04
PR: #81
File: tests/core-providers/scenarios/end_to_end_tool_calling.go:43-45
Timestamp: 2025-06-16T04:13:55.437Z
Learning: In the Bifrost codebase, errors returned from client methods like ChatCompletionRequest are of type BifrostError, not the standard error interface. For testing these errors, use require.Nilf instead of require.NoErrorf since BifrostError doesn't work with the standard error assertion methods.

Learnt from: Pratham-Mishra04
PR: #81
File: tests/core-providers/scenarios/simple_chat.go:39-41
Timestamp: 2025-06-16T04:13:42.755Z
Learning: In the Bifrost codebase, errors returned from methods like ChatCompletionRequest are of type BifrostError (a custom error type) rather than the standard Go error interface. Therefore, require.Nilf should be used for error assertions instead of require.NoErrorf.

Learnt from: Pratham-Mishra04
PR: #83
File: core/providers/mistral.go:96-101
Timestamp: 2025-06-15T13:50:41.418Z
Learning: In the Bifrost project, when a provider doesn't support a specific operation (like text completion), the IsBifrostError flag should be set to false to mark it as a provider-level error rather than a Bifrost framework error. This is intentional design for unsupported operations.

Learnt from: Pratham-Mishra04
PR: #65
File: transports/bifrost-http/integrations/utils.go:169-173
Timestamp: 2025-06-09T17:33:52.234Z
Learning: The ChatCompletionRequest method in the Bifrost client follows a contract where the result parameter will never be nil if the error parameter is nil. This means when error checking passes (err == nil), the result is guaranteed to be valid and can be safely used without additional nil checks.

Learnt from: Pratham-Mishra04
PR: #152
File: transports/bifrost-http/plugins/logging/utils.go:94-111
Timestamp: 2025-07-10T13:44:23.297Z
Learning: Pratham-Mishra04 prefers not to add error handling for JSON marshaling operations in the Bifrost logging plugin (transports/bifrost-http/plugins/logging/utils.go) because logging is not critical functionality and the structured schema data being marshaled is unlikely to fail. They accept the risk of not handling json.Marshal errors in logging contexts to keep the code simple.

Learnt from: Pratham-Mishra04
PR: #54
File: core/schemas/bifrost.go:46-49
Timestamp: 2025-06-04T09:22:18.123Z
Learning: In core/schemas/bifrost.go, the RequestInput struct uses ChatCompletionInput *[]BifrostMessage (pointer-to-slice) rather than []BifrostMessage to properly represent union type semantics. For text completion requests, ChatCompletionInput should be nil to indicate "no chat payload at all", while for chat completion requests it should be non-nil (even if empty slice). This distinguishes between different request types rather than just empty vs non-empty chat messages.

Learnt from: Pratham-Mishra04
PR: #94
File: core/bifrost.go:823-845
Timestamp: 2025-06-18T15:15:51.323Z
Learning: In the Bifrost project, the team prioritizes maintaining consistent error handling patterns over exposing detailed error context. All errors should be wrapped in the standard BifrostError structure rather than creating specific error types or exposing richer error details like exit codes or stderr output.

Learnt from: Pratham-Mishra04
PR: #55
File: core/providers/anthropic.go:526-550
Timestamp: 2025-06-04T09:29:46.287Z
Learning: In core/providers/anthropic.go, the content field in formattedMessages is always of type []interface{} because it's explicitly constructed that way upstream in the prepareAnthropicChatRequest function. Defensive type casting for multiple types is not needed since the type is guaranteed by the construction logic.

Learnt from: Pratham-Mishra04
PR: #162
File: transports/bifrost-http/integrations/genai/types.go:0-0
Timestamp: 2025-07-16T07:13:29.496Z
Learning: Pratham-Mishra04 prefers to avoid redundant error handling across architectural layers in the Bifrost streaming implementation. When error handling (such as timeouts, context cancellation, and JSON marshaling failures) is already handled at the provider level, they prefer not to duplicate this logic at the transport integration layer to keep the code simple and avoid unnecessary complexity.

Learnt from: Pratham-Mishra04
PR: #81
File: tests/core-providers/scenarios/complete_end_to_end.go:39-41
Timestamp: 2025-06-16T04:12:05.427Z
Learning: In the Bifrost system, error returns are of type BifrostError rather than the standard Go error interface. Therefore, use require.Nilf(t, err, ...) instead of require.NoError(t, err) when checking for errors in Bifrost function calls.

core/providers/ollama.go (10)