Merged
Conversation
|
|
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughExports Anthropic schema normalization, adjusts Bedrock streaming checksum error handling and structured-output signaling, adds LogLevel to routing engine logs and updates callers, and minor test and logging updates across governance plugin files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
Confidence Score: 5/5Safe to merge; only P2 suggestions remain around log-level classification. No P0 or P1 findings. The two P2 comments are style-level log-level classifications (Info vs Warn) that do not affect correctness. Core Bedrock changes are well-guarded, the No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "bedrock cli compatibility changes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
core/schemas/context.go (1)
407-438:⚠️ Potential issue | 🟠 MajorReset and delegate scoped contexts before pooling them.
WithPluginScopealiasesscoped.doneto the root context, but the scoped instance keeps its own cancel state, andReleasePluginScopeonly clears a subset of fields beforepluginScopePool.Put. A plugin callingCancel()on the scoped context can close the shared root channel through the wrongdoneOnce, and reused scopes can retain stale state across requests. Delegate cancellation tovalueDelegatefor scoped contexts and zero the whole struct before returning it to the pool. As per coding guidelines "Reset all fields of pooled objects before calling pool.Put() to prevent data leakage between requests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/context.go` around lines 407 - 438, WithPluginScope currently aliases scoped.done to the root context and ReleasePluginScope only clears some fields before returning the scoped context to pluginScopePool, which can leak cancel state and other data; update WithPluginScope so scoped contexts delegate cancellation to valueDelegate (do not reuse or alias done/doneOnce from the root), and change ReleasePluginScope to fully zero out all fields on the scoped BifrostContext (parent, done, doneOnce, pluginScope, pluginLogs, valueDelegate and any other struct fields) before calling pluginScopePool.Put to ensure no stale state is retained and cancellation is always routed through valueDelegate.
🤖 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/utils.go`:
- Around line 86-89: The current appendAnthropicBetaToFields call (using
anthropic.AnthropicStructuredOutputsBetaHeader and
bedrockReq.AdditionalModelRequestFields) adds the structured-output beta but
later code calls Set("anthropic_beta", betaHeaders) which overwrites any
existing anthropic_beta entries and can drop the structured-output beta when
server-tool betas are also added; change the logic where anthropic_beta is set
so you merge with any existing anthropic_beta entries in
bedrockReq.AdditionalModelRequestFields (read the existing value, combine
arrays/lists, deduplicate entries) and then write the merged list back via
Set("anthropic_beta", mergedBetaHeaders) so both structured-output and
server-tool betas are preserved (update code paths around
appendAnthropicBetaToFields and the Set("anthropic_beta", ...) call).
In `@plugins/governance/main.go`:
- Around line 903-906: The routing-rule evaluation failure is being logged to
the routing-engine logs at Info level; change the severity to an error level so
consumers can distinguish hard failures. Update the call to
ctx.AppendRoutingEngineLog in the error branch (where p.logger.Error is invoked)
to use schemas.LogLevelError (or the appropriate error enum) instead of
schemas.LogLevelInfo, keeping the same message (fmt.Sprintf("Routing rule
evaluation error: %v", err)) and the same routing key
schemas.RoutingEngineRoutingRule.
In `@plugins/governance/routing.go`:
- Around line 474-523: buildNoMatchContext currently writes raw header/param
values (from variables["headers"/"params"]) into logs; change it to never log
actual values — for each key found by extractMapKeysFromCEL, append only
presence/missing or a redacted placeholder (e.g., key=<present> or
key=<redacted> / key=<missing>) instead of fmt.Sprintf("%s=%q", k, v); update
the block that iterates m := variables[mapName].(map[string]string) to replace v
with a constant redaction string or a presence marker and keep the existing
"key=<missing>" for absent keys so logs contain no PII/secrets while still
showing which keys were referenced.
---
Outside diff comments:
In `@core/schemas/context.go`:
- Around line 407-438: WithPluginScope currently aliases scoped.done to the root
context and ReleasePluginScope only clears some fields before returning the
scoped context to pluginScopePool, which can leak cancel state and other data;
update WithPluginScope so scoped contexts delegate cancellation to valueDelegate
(do not reuse or alias done/doneOnce from the root), and change
ReleasePluginScope to fully zero out all fields on the scoped BifrostContext
(parent, done, doneOnce, pluginScope, pluginLogs, valueDelegate and any other
struct fields) before calling pluginScopePool.Put to ensure no stale state is
retained and cancellation is always routed through valueDelegate.
🪄 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: 2baa88dd-76de-4d75-bee9-49d652a6e215
📒 Files selected for processing (11)
core/providers/anthropic/utils.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/bifrost.gocore/schemas/context.goplugins/governance/allowonallvirtualkeys_test.goplugins/governance/httptransportprehook_test.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/storeconcurrency_test.go
💤 Files with no reviewable changes (1)
- plugins/governance/storeconcurrency_test.go
This was referenced Apr 26, 2026
Merged
akshaydeo
force-pushed
the
04-26-bedrock_cli_compatibility_changes
branch
from
3198154 to
d9409e8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/schemas/bifrost.go (1)
316-321: Update theRoutingEngineLogEntryformat comment to includelevel.The comment still describes
[timestamp] [engine] - message, butLevelis now part of the schema.Proposed comment update
-// format: [timestamp] [engine] - message +// format: [timestamp] [engine] [level] - message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/bifrost.go` around lines 316 - 321, Update the top-of-struct format comment for RoutingEngineLogEntry to reflect the added Level field — change the example format from "[timestamp] [engine] - message" to include level (for example "[timestamp] [engine] [level] - message") and ensure the comment mentions that Level corresponds to the LogLevel field; locate the comment immediately above the RoutingEngineLogEntry type declaration and adjust the human-readable example and description to include `Level` alongside `Engine`, `Timestamp`, and `Message`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/schemas/bifrost.go`:
- Around line 316-321: Update the top-of-struct format comment for
RoutingEngineLogEntry to reflect the added Level field — change the example
format from "[timestamp] [engine] - message" to include level (for example
"[timestamp] [engine] [level] - message") and ensure the comment mentions that
Level corresponds to the LogLevel field; locate the comment immediately above
the RoutingEngineLogEntry type declaration and adjust the human-readable example
and description to include `Level` alongside `Engine`, `Timestamp`, and
`Message`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 069a3e1a-4907-4218-90e9-dfacee3041e8
📒 Files selected for processing (11)
core/providers/anthropic/utils.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/bifrost.gocore/schemas/context.goplugins/governance/allowonallvirtualkeys_test.goplugins/governance/httptransportprehook_test.goplugins/governance/main.goplugins/governance/routing.goplugins/governance/storeconcurrency_test.go
💤 Files with no reviewable changes (1)
- plugins/governance/storeconcurrency_test.go
✅ Files skipped from review due to trivial changes (1)
- plugins/governance/main.go
🚧 Files skipped from review as they are similar to previous changes (5)
- core/providers/anthropic/utils.go
- core/providers/bedrock/bedrock.go
- core/schemas/context.go
- core/providers/bedrock/utils.go
- plugins/governance/routing.go
akshaydeo
force-pushed
the
04-26-bedrock_cli_compatibility_changes
branch
from
d9409e8 to
867280f
Compare
Contributor
Author
Merge activity
|
18 tasks
akshaydeo
added a commit
that referenced
this pull request
May 1, 2026
## Summary Fixes a user-reported regression where Claude Opus 4.7 structured output requests on Bedrock Converse failed with `output_config.format: Extra inputs are not permitted`. The root cause was that PR #3053 routed Anthropic-on-Bedrock structured outputs through `output_config.format` + `anthropic_beta: structured-outputs-2025-11-13` in `additionalModelRequestFields`, which Bedrock Converse rejects for certain Claude variants (including Opus 4.7). The fix routes all Bedrock + Anthropic structured output requests through the synthetic `bf_so_*` tool path (the same path used by non-Anthropic Bedrock models), which is a standard Converse tool call accepted by all Claude variants. ## Changes - `convertResponseFormatToTool` and `convertTextFormatToTool` in `core/providers/bedrock/utils.go` no longer branch on `IsAnthropicModel` to emit `output_config.format`; both functions now fall through to the `bf_so_*` synthetic-tool synthesis for all Bedrock models. - `ToBedrockResponsesRequest` in `core/providers/bedrock/responses.go` drops the `output_config.format` + `anthropic_beta` injection block that was added in #3053. - `convertChatParameters` in `core/providers/bedrock/utils.go` similarly drops the `output_config.format` + `anthropic_beta` injection block. - Added `TestBedrockAnthropicChatStructuredOutputUsesSyntheticTool` and `TestToBedrockResponsesRequest_AnthropicStructuredOutputUsesSyntheticTool` unit tests that assert `output_config` and the structured-outputs beta header are absent, and that the `bf_so_*` tool is injected and forced via `tool_choice`. - Added a live end-to-end regression repro test `BedrockOpus47Tests/TestBedrockOpus47StructuredOutputRegression` (skipped unless `BEDROCK_OPUS_47_MODEL_ID` is set) that mirrors the exact user-reported Python snippet through the same wire path. - Helm chart bumped to `2.1.13`; `enforceAuthOnInference` surfaced as a commented default in `values.yaml`; `enforceGovernanceHeader` marked deprecated. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh # Unit tests covering the synthetic-tool routing fix go test ./core/providers/bedrock/... -run TestBedrockAnthropicChatStructuredOutputUsesSyntheticTool go test ./core/providers/bedrock/... -run TestToBedrockResponsesRequest_AnthropicStructuredOutputUsesSyntheticTool # Live Opus 4.7 regression repro (requires AWS credentials + Bedrock entitlement) export BEDROCK_OPUS_47_MODEL_ID=anthropic.claude-opus-4-7-v1:0 # or your inference-profile id make test-core PROVIDER=bedrock TESTCASE=TestBedrockOpus47StructuredOutputRegression ``` The live repro test sends a structured output request with `output_config.format` (json_schema with `anyOf`-style nullable fields) and no outer `anthropic-beta` header — the exact shape of the user's failing request. A passing run logs `Bedrock Opus 4.7 structured-output request SUCCEEDED`. ## Breaking changes - [ ] Yes - [x] No ## Related issues Regression introduced in #3053 (commit `7df13ab45`). ## Security considerations No auth, secrets, or PII changes. The `.infisical` directory is now gitignored to prevent accidental credential leakage. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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