fix: return raw HTML in error responses instead of extracting messages #1128
Conversation
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughError handling for HTML responses was changed: providers now set a fixed HTML-marker message (ErrProviderResponseHTML) and attach the raw response body as the error value instead of calling ExtractHTMLErrorMessage(). The ExtractHTMLErrorMessage function is preserved but marked UNUSED in utils. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/providers/utils/utils.go (1)
593-664: Note: Unused function retained for potential future use.The
ExtractHTMLErrorMessagefunction is now marked as unused but preserved with its full implementation. While the comment indicates it "could be useful in the future," consider:
- If there's a concrete plan to use this function (e.g., making HTML extraction optional via configuration), the retention is reasonable
- If the need is speculative, removing it and relying on git history might reduce maintenance burden
- The function has good ReDoS protection with
maxBodySizelimit, so it's safe if re-introduced later
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/providers/mistral/mistral.go(1 hunks)core/providers/openai/openai.go(1 hunks)core/providers/utils/utils.go(3 hunks)core/providers/vertex/errors.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/utils/utils.gocore/providers/openai/openai.gocore/providers/mistral/mistral.gocore/providers/vertex/errors.go
🧠 Learnings (3)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/utils/utils.gocore/providers/openai/openai.gocore/providers/mistral/mistral.gocore/providers/vertex/errors.go
📚 Learning: 2025-12-11T11:58:25.307Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: core/providers/openai/responses.go:42-84
Timestamp: 2025-12-11T11:58:25.307Z
Learning: In core/providers/openai/responses.go (and related OpenAI response handling), document and enforce the API format constraint: if ResponsesReasoning != nil and the response contains content blocks, all content blocks should be treated as reasoning blocks by default. Implement type guards or parsing logic accordingly, and add unit tests to verify that when ResponsesReasoning is non-nil, content blocks are labeled as reasoning blocks. Include clear comments in the code explaining the rationale and ensure downstream consumers rely on this behavior.
Applied to files:
core/providers/openai/openai.go
📚 Learning: 2025-12-14T14:43:30.902Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 980
File: core/providers/openai/images.go:10-22
Timestamp: 2025-12-14T14:43:30.902Z
Learning: Enforce the OpenAI image generation SSE event type values across the OpenAI image flow in the repository: use "image_generation.partial_image" for partial chunks, "image_generation.completed" for the final result, and "error" for errors. Apply this consistently in schemas, constants, tests, accumulator routing, and UI code within core/providers/openai (and related Go files) to ensure uniform event typing and avoid mismatches.
Applied to files:
core/providers/openai/openai.go
🔇 Additional comments (4)
core/providers/vertex/errors.go (1)
55-71: LGTM! Raw HTML error responses now returned for better debugging.The change to return the full HTML body as the error message (instead of attempting to extract a specific message) improves debugging capabilities for provider errors. The implementation correctly handles status codes and metadata.
Note: HTML error pages from providers can be verbose, but this trade-off is acceptable for debugging purposes as noted in the PR description.
core/providers/mistral/mistral.go (1)
322-332: LGTM! Consistent HTML error handling in transcription.The change to return the raw HTML response body as the error message is consistent with the PR's approach across all providers. The error structure is correctly populated and uses the copied response body appropriately.
core/providers/openai/openai.go (1)
2006-2017: LGTM! Consistent HTML error handling implementation.The change to return the full HTML response body aligns with the PR's objective to improve debugging by providing complete error information from providers. The error structure and response handling are correct.
core/providers/utils/utils.go (1)
359-367: LGTM! Consistent raw HTML error handling in utility function.The change to return the full decoded HTML body as the error message is consistent with the changes across all provider implementations. This provides better debugging information when providers return HTML error pages.
Pratham-Mishra04
force-pushed
the
12-18-chore_send_back_full_html_instead
branch
from
cc46374 to
5157d2a
Compare
Pratham-Mishra04
force-pushed
the
12-18-chore_send_back_full_html_instead
branch
from
5157d2a to
511efc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx (1)
53-111: These UI styling changes appear unrelated to the PR's stated objective.The PR objective describes fixing error handling for HTML responses in Go providers, but this file contains only React UI styling updates (text color tokens, className ordering, and minor wording adjustments).
Per the coding guidelines to "always see all changes in the light of the whole stack of PRs," these changes may belong to a different PR in the stack. Please verify:
- Are these changes intended for a separate PR in the stack?
- If they belong in this PR, the PR description should be updated to reflect UI styling changes.
Technical assessment of the changes:
The styling updates themselves are fine—migrating fromtext-gray-600totext-muted-foregroundpromotes design token consistency, and thereadmeLinkprop is correctly used (ContactUsView already accepts this prop per the codebase).
🧹 Nitpick comments (1)
core/providers/vertex/errors.go (1)
56-72: LGTM! HTML error handling aligns with PR objectives.The change correctly implements the new pattern of returning raw HTML responses with a fixed message constant. This provides better debugging information and maintains consistency with error handling in other provider files (Mistral, OpenAI).
The implementation properly:
- Uses the
ErrProviderResponseHTMLconstant for the message- Wraps the raw HTML body as an error for full context
- Preserves the error structure with status code and extra fields
💡 Optional: Consider HTML response size limits
For edge cases where providers return very large HTML error pages, you might want to consider truncating the HTML body to prevent excessive memory usage or log bloat:
const maxHTMLErrorBodySize = 10 * 1024 // 10KB htmlBody := string(decodedBody) if len(htmlBody) > maxHTMLErrorBodySize { htmlBody = htmlBody[:maxHTMLErrorBodySize] + "... (truncated)" } Error: errors.New(htmlBody),This is purely optional and only relevant if you observe issues with large HTML error responses in practice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/providers/mistral/mistral.go(1 hunks)core/providers/openai/openai.go(1 hunks)core/providers/utils/utils.go(3 hunks)core/providers/vertex/errors.go(2 hunks)ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/providers/utils/utils.go
- core/providers/mistral/mistral.go
- core/providers/openai/openai.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/providers/vertex/errors.goui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/providers/vertex/errors.go
🧬 Code graph analysis (2)
core/providers/vertex/errors.go (1)
core/schemas/provider.go (1)
ErrProviderResponseHTML(32-32)
ui/app/_fallbacks/enterprise/components/api-keys/APIKeysView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(16-47)
🔇 Additional comments (1)
core/providers/vertex/errors.go (1)
4-4: LGTM! Import is necessary and correctly placed.The addition of the standard
errorspackage is required for theerrors.New()call on line 62 and follows proper import conventions.
Summary
Improved error handling for HTML responses by returning the full HTML body instead of attempting to extract specific error messages.
Changes
ExtractHTMLErrorMessagefunction but marked it as unused for potential future useType of change
Affected areas
How to test
Test error scenarios where providers return HTML responses:
Breaking changes
Security considerations
The change now returns full HTML responses in error messages, which could potentially include more information than before, but this is necessary for proper debugging of provider errors.
Checklist