Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoved provider-name prefixing from several stream error messages; documented per-client MCP header forwarding ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (3 warnings, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5Safe to merge — all findings are P2 style suggestions with no blocking correctness issues. The only finding is a P2 edge case in the allowed_extra_headers input where Enter-key form submission could miss in-flight edits; the primary button-click path works correctly. All other changes (docs, VK details sheet, Go provider fixes) are clean. ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx — onBlur-only form field update for allowed_extra_headers
|
| Filename | Overview |
|---|---|
| ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx | Adds Allowed Extra Headers UI field; uses onBlur-only pattern for parsing the comma-separated input into the form array, which can miss updates on Enter-key submission. |
| ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx | New read-only detail sheet showing VK status, provider configs, MCP client configs, budgets, and rate limits; implementation looks correct. |
| ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx | Minor updates to MCP config section and VK form; wildcard tool selection logic is correct. |
| transports/bifrost-http/handlers/oauth2_consent.go | OAuth per-user consent flow handler; input is properly HTML-escaped, flow expiry and browser-secret validation are applied consistently across all endpoints. |
| docs/mcp/connecting-to-servers.mdx | New Forwarding Request Headers to MCP Servers section documenting allowed_extra_headers; content is accurate and matches implementation. |
| core/providers/bedrock/bedrock.go | Minor fix to Bedrock provider; transport, signing, and retry logic appear correct. |
| core/providers/openai/openai.go | Minor fix to OpenAI provider; client setup, URL construction, and store disable logic look correct. |
Reviews (2): Last reviewed commit: "fix: minor fixes and doc additions" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
core/providers/bedrock/bedrock.go (2)
1228-1242:⚠️ Potential issue | 🟡 MinorError message format inconsistent with
TextCompletionStream.This error message no longer includes the provider name prefix (
"stream %s: %s"), whileTextCompletionStreamat line 1000 still uses"%s stream %s: %s"withproviderName. The retryable error at line 996 inTextCompletionStreamalso still includes the provider name in the message.This inconsistency affects both logging/debugging and error message formats returned to callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1228 - 1242, The error message created in the streaming error path uses fmt.Errorf("stream %s: %s", excType, errMsg) and thus omits the provider name, causing inconsistency with TextCompletionStream which formats messages as "%s stream %s: %s"; update the error construction in the streaming path to include the provider name prefix (same format used by TextCompletionStream) so both the retryableBedrockExceptions branch (where ProcessAndSendBifrostError is called) and the else branch (ProcessAndSendError) emit messages like "%s stream %s: %s" using the same provider identifier (e.g., providerName or provider.Name) to keep logging and returned error formats consistent.
1572-1600:⚠️ Potential issue | 🟡 MinorSame inconsistency as
ChatCompletionStream.These changes mirror the
ChatCompletionStreamchanges but leaveTextCompletionStreamwith the old format including provider name. All three streaming methods should use the same error formatting approach for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/bedrock/bedrock.go` around lines 1572 - 1600, The TextCompletionStream error formatting is inconsistent with ChatCompletionStream: locate the code that builds the error from message.Headers (symbols: message.Headers, excType, errMsg) inside the TextCompletionStream handling and change the error construction so it does not prepend the provider/stream label; format the error as "<exception-type>: <message>" (same approach as ChatCompletionStream) before passing it to providerUtils.ProcessAndSendError/ProcessAndSendBifrostError so all three streaming methods use the same error format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/bedrock/bedrock.go`:
- Line 1200: The warning in the EventStream log currently omits the provider
context; update the log call at provider.logger.Warn("Error decoding EventStream
message: %v", err) to include the provider name (e.g.,
provider.logger.Warn("error decoding %s EventStream message: %v", providerName,
err)) so it matches the format used in TextCompletionStream, and review the
other streaming error logs (TextCompletionStream and the similar messages around
the existing decode errors) to ensure all streaming error logs consistently
include providerName and use the same message casing/format.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 324-327: The current new client creation sets tools_to_execute:
["*"] which grants full tool access by default; change newConfig (the object
with mcp_client_name and tools_to_execute in virtualKeySheet.tsx) to use a
deny-by-default value (e.g., an empty array or omit tools_to_execute) so newly
created MCP clients have no tools until explicitly selected, and update any
downstream usage that expects tools_to_execute by treating empty/undefined as
"no permissions" rather than "all permissions."
---
Outside diff comments:
In `@core/providers/bedrock/bedrock.go`:
- Around line 1228-1242: The error message created in the streaming error path
uses fmt.Errorf("stream %s: %s", excType, errMsg) and thus omits the provider
name, causing inconsistency with TextCompletionStream which formats messages as
"%s stream %s: %s"; update the error construction in the streaming path to
include the provider name prefix (same format used by TextCompletionStream) so
both the retryableBedrockExceptions branch (where ProcessAndSendBifrostError is
called) and the else branch (ProcessAndSendError) emit messages like "%s stream
%s: %s" using the same provider identifier (e.g., providerName or provider.Name)
to keep logging and returned error formats consistent.
- Around line 1572-1600: The TextCompletionStream error formatting is
inconsistent with ChatCompletionStream: locate the code that builds the error
from message.Headers (symbols: message.Headers, excType, errMsg) inside the
TextCompletionStream handling and change the error construction so it does not
prepend the provider/stream label; format the error as "<exception-type>:
<message>" (same approach as ChatCompletionStream) before passing it to
providerUtils.ProcessAndSendError/ProcessAndSendBifrostError so all three
streaming methods use the same error format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 384a208d-d20a-465f-aa19-56afb412c63f
⛔ Files ignored due to path filters (1)
docs/media/ui-mcp-allowed-extra-headers.pngis excluded by!**/*.png
📒 Files selected for processing (11)
core/providers/bedrock/bedrock.gocore/providers/openai/openai.godocs/mcp/connecting-to-servers.mdxdocs/openapi/openapi.jsondocs/providers/aliasing-models.mdxdocs/providers/request-options.mdxdocs/providers/routing-rules.mdxtransports/bifrost-http/handlers/providers.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
💤 Files with no reviewable changes (1)
- transports/bifrost-http/handlers/providers.go
danpiths
changed the base branch from
04-07-fix_oauth_user_level_oauth_fixes
to
graphite-base/2562
|
|
Merge activity
|
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
Pratham-Mishra04
force-pushed
the
04-08-fix_minor_fixes_and_doc_additions
branch
from
4bceac5 to
0a0cb9b
Compare
3 participants
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines