fix: strip cache point from bedrock requests for models which do not support it - #3754
Merged
akshaydeo merged 2 commits intoMay 26, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughConditionally strip Bedrock CachePoint fields for models that don't support prompt-caching (non-Anthropic/Nova) by detecting model capability, mutating requests to remove CachePoint entries, integrating the strip into chat/responses converters, and adding unit tests covering message, system, nested tool, and tool-config cases. ChangesBedrock Cache Point Conditional Stripping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Confidence Score: 4/5Safe to merge once the empty-message edge case is addressed; the stripping logic is otherwise correct and well-tested. The stripping loop in stripCachePointsFromBedrockRequest does not remove a message when all of its content blocks are cache points. After stripping, that message ends up with an empty Content slice, which the Bedrock Converse API will reject with a 400 — trading one failure mode for another. This is an unlikely but real scenario for callers migrating payloads built for Anthropic models to GLM/Llama models. core/providers/bedrock/utils.go — the inner stripping loop should also drop messages whose content becomes empty after filtering. Important Files Changed
Reviews (3): Last reviewed commit: "fix: strip cache point from bedrock requ..." | 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.
🧹 Nitpick comments (1)
core/providers/bedrock/utils.go (1)
2195-2237: ⚡ Quick winAdd focused tests for nested/system cache-point stripping paths.
This helper now owns several in-place compaction branches; a small table-driven unit test matrix would protect against regressions (nested
ToolResult.Content, cache-point-only system entries, and mixed tool configs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/bedrock/utils.go` around lines 2195 - 2237, Add table-driven unit tests for stripCachePointsFromBedrockRequest covering the in-place compaction branches: include cases with nested ToolResult.Content containing some CachePoint entries to ensure inner arrays are compacted, messages with mixed CachePoint and non-CachePoint content, system entries that are cache-point-only (should be removed) and system entries with text/guard retained, and ToolConfig.Tools with CachePoint fields to verify they are nilled; use BedrockConverseRequest fixtures exercising Messages, System, and ToolConfig.Tools and assert post-call slices/lengths and that all CachePoint fields are cleared as expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/providers/bedrock/utils.go`:
- Around line 2195-2237: Add table-driven unit tests for
stripCachePointsFromBedrockRequest covering the in-place compaction branches:
include cases with nested ToolResult.Content containing some CachePoint entries
to ensure inner arrays are compacted, messages with mixed CachePoint and
non-CachePoint content, system entries that are cache-point-only (should be
removed) and system entries with text/guard retained, and ToolConfig.Tools with
CachePoint fields to verify they are nilled; use BedrockConverseRequest fixtures
exercising Messages, System, and ToolConfig.Tools and assert post-call
slices/lengths and that all CachePoint fields are cleared as expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4f651ed-3b3b-4ff7-b6f7-3f8dbe80abc6
📒 Files selected for processing (4)
core/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/utils.go
TejasGhatte
force-pushed
the
05-26-fix_strip_cache_point_from_bedrock_requests_for_models_which_do_not_support_it
branch
from
May 26, 2026 08:45
4a5a293 to
469b338
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/providers/bedrock/cache_points_test.go (1)
126-145: ⚡ Quick winAssert the non-cache tool remains unchanged in the tool-config test.
This test verifies cache-point zeroing, but it doesn’t currently prove the real tool spec was preserved as-is.
Suggested test hardening
tools := req.ToolConfig.Tools if len(tools) != 2 { t.Fatalf("expected 2 tools (cache point zeroed, not removed), got %d", len(tools)) } + if tools[0].ToolSpec == nil || tools[0].ToolSpec.Name != "get_weather" { + t.Fatalf("expected first tool spec to be preserved, got %+v", tools[0]) + } if tools[1].CachePoint != nil { t.Errorf("expected tool CachePoint to be nil, got %+v", tools[1].CachePoint) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/bedrock/cache_points_test.go` around lines 126 - 145, The test TestStripCachePoints_ToolConfigCachePoints should also assert the non-cache tool spec is unchanged: before calling stripCachePointsFromBedrockRequest capture the original tool spec (e.g., req.ToolConfig.Tools[0].ToolSpec or its Name), then after the call assert that req.ToolConfig.Tools[0].ToolSpec (or .Name) still equals the original value; this ensures stripCachePointsFromBedrockRequest preserves non-cache tools while only nil-ing CachePoint on the cache tool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/bedrock/utils.go`:
- Around line 2232-2236: After clearing CachePoint on each entry, filter out any
tool entries that became empty (cache-point-only placeholders) so they don't get
sent to models that don't support them; iterate req.ToolConfig.Tools and rebuild
the slice keeping only tools that have any meaningful field set (e.g.,
name/type/ID or other non-zero fields) rather than only CachePoint, or add an
isEmptyTool helper to detect and drop wholly-empty tool structs before leaving
req.ToolConfig.Tools. Ensure you perform this filtering immediately after the
loop that sets CachePoint = nil.
---
Nitpick comments:
In `@core/providers/bedrock/cache_points_test.go`:
- Around line 126-145: The test TestStripCachePoints_ToolConfigCachePoints
should also assert the non-cache tool spec is unchanged: before calling
stripCachePointsFromBedrockRequest capture the original tool spec (e.g.,
req.ToolConfig.Tools[0].ToolSpec or its Name), then after the call assert that
req.ToolConfig.Tools[0].ToolSpec (or .Name) still equals the original value;
this ensures stripCachePointsFromBedrockRequest preserves non-cache tools while
only nil-ing CachePoint on the cache tool.
🪄 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: b12bef33-64e9-4a64-99ef-499137cef047
📒 Files selected for processing (5)
core/providers/bedrock/cache_points_test.gocore/providers/bedrock/chat.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/schemas/utils.go
akshaydeo
previously approved these changes
May 26, 2026
TejasGhatte
force-pushed
the
05-26-fix_strip_cache_point_from_bedrock_requests_for_models_which_do_not_support_it
branch
from
May 26, 2026 08:57
469b338 to
11b3319
Compare
Contributor
Merge activity
|
akshaydeo
changed the base branch from
05-26-fix_skip_empty_text_block_in_bedrock
to
graphite-base/3754
May 26, 2026 10:15
akshaydeo
dismissed stale reviews from coderabbitai[bot] and themself
May 26, 2026 10:16
The base branch was changed.
akshaydeo
approved these changes
May 26, 2026
akshaydeo
deleted the
05-26-fix_strip_cache_point_from_bedrock_requests_for_models_which_do_not_support_it
branch
May 26, 2026 10:24
akshaydeo
pushed a commit
that referenced
this pull request
May 26, 2026
…support it (#3754) ## Summary Some Bedrock models (e.g. GLM, Llama) do not support prompt-caching cache points in the Converse API and will return a 400 error if cache point blocks are present in the request. This PR adds a guard that strips cache points from Bedrock requests before they are sent, for any model that does not support them. ## Changes - Added `BedrockModelSupportsCachePoints` in `core/schemas/utils.go` that returns `true` only for Anthropic and Nova models, which are the models known to support explicit cache points in the Converse API. - Added `stripCachePointsFromBedrockRequest` in `core/providers/bedrock/utils.go` that removes cache point blocks from message content, nested tool result content, system messages, and tool config entries. - Called `stripCachePointsFromBedrockRequest` in both `ToBedrockChatCompletionRequest` and `ToBedrockResponsesRequest` when the target model does not support cache points, preventing 400 errors from unsupported models receiving cache point blocks. ## 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 Send a Bedrock Converse request to a non-Anthropic, non-Nova model (e.g. a GLM or Llama model) with cache point blocks included in the request body. Verify the request succeeds without a 400 error and that cache point blocks are absent from the forwarded request. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Security considerations No security implications. Cache point blocks are stripped only from the outbound request payload for unsupported models and do not affect authentication, secrets, or PII handling. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
Merged
akshaydeo
added a commit
that referenced
this pull request
May 26, 2026
## ✨ Features
- **Azure v1 API Migration** — Migrated Azure provider to the v1 API:
removed the `api-version` query parameter and the
`/openai/deployments/{model}/...` URL pattern in favor of
`/openai/v1/{operation}`; the `api_version` field has been dropped from
`AzureKeyConfig` (#3661, #3756)
- **EnvVar Support for OTEL & Prometheus Configs** — `CollectorURL`,
`MetricsEndpoint`, headers, push gateway URL, and basic auth credentials
can now be sourced from environment variables (e.g.,
`env.OTEL_COLLECTOR_URL`); added a new `ConfigMarshallerPlugin`
interface that lets plugins control storage/redaction round-trips
(#3651)
- **OTel Extra Header Forwarding** — `x-bf-eh-*` extra headers forwarded
to upstream providers are now also emitted on the request span under
`gen_ai.request.extra_header.*` for end-to-end tracing (#3730)
- **OTel Semantic Conventions** — Aligned OTel attribute keys with the
OpenTelemetry GenAI spec (canonical `gen_ai.*` and new `bifrost.*`
attributes); legacy attributes are retained in parallel to avoid
breaking existing dashboards (#3732)
- **VK Quota with Provider Configs** — `GetVirtualKeyQuotaByValue` and
the `getVirtualKeyQuota` HTTP response now include `provider_configs`
with their budgets and rate limits (#3721)
- **MCP Temp Token Non-Auth Toggle** — Added
`mcp_enable_temp_token_auth` client config flag to gate short-lived MCP
token minting for non-authenticated users (#3720)
- **Responses Stream in JSON Parser** — `jsonparser` plugin now handles
OpenAI Responses API streaming (`ResponsesStreamRequest`) in addition to
chat completions (#3749)
- **Session API Rework** — Logout now calls both the password-based
session logout and OAuth logout endpoints and resets all RTK Query cache
state (#3698)
## 🐞 Fixed
- **Streaming Latency for Observability** — Deferred root span
termination to the trace completer callback for streaming requests so
request latency is no longer inflated by header-flush time (#3762)
- **Stream Cancellation Race** — Set `BifrostContextKeyConnectionClosed`
before closing the stream and short-circuit `idleTimeoutReader.Read`
when the connection is already closed to avoid panics and hangs on
cancellation (#3733)
- **Bedrock Cache Points** — Strip cache points from Bedrock requests
for models that do not support prompt caching (e.g., GLM, Llama) to
avoid Converse API errors (#3754)
- **Bedrock Empty Text Blocks** — Skip empty/nil text blocks during
Bedrock response conversion to avoid invalid messages (#3747)
- **Bedrock Reasoning + Tools** — Preserve reasoning content blocks on
assistant turns that also contain tool calls in the Bedrock chat
converter (#3690)
- **Bedrock Search Content & Video** — Restored search content and video
parts that were being dropped from Bedrock-native passthrough requests
(#3729)
- **Structured Output Stop Reason** — Fixed an incorrect `tool_calls`
finish reason when structured output is combined with extended-thinking
tools (#3685)
- **Gemini Tool Schema Passthrough** — Forward full tool parameter
schemas via `parametersJsonSchema` instead of the lossy `parameters`
form; corrected tool response role to `user`; resolved structured output
+ tools conflict (#3761)
- **Anthropic Stop Reason & Tool Versions** — Normalized stop reason
mapping (`end_turn` to `stop`, `tool_use` to `tool_calls`, `max_tokens`
to `length`) and upgraded `text_editor_20250124`/`str_replace_editor` to
`text_editor_20250728` for computer-use tools (#3761)
- **Azure Endpoint Redaction** — Fixed a panic when
`AzureKeyConfig.Endpoint` is a literal value rather than an env
reference (#3761)
- **Auth Middleware Path Match** — Match temp-token auth middleware
whitelist against the request path only, not the full URI with query
parameters (#3737)
- **Governance Blocked Models UI** — Restored the missing Blocked Models
create/edit UI in the VK provider config sheet (#3750)
- **Logging Plugin Cleanup Drain** — Fixed a shutdown race where
`batchWriter` could drop in-flight log entries; `Cleanup` now drains
both the recovered batch and remaining queue within a 30-second budget
(#3717)
- **Model Rankings Empty Entries** — Excluded entries with empty `model`
values from model rankings matview queries so blank rows no longer
surface in the UI (#3758)
- **User Filter Duplicates** — Recreated `mv_filter_users` matview to
require non-empty `user_name`, eliminating duplicate filter dropdown
entries (#3764)
- **User Filter Display Name** — Use `user_name` instead of `user_id` as
the display label for users in logging filters (#3691)
- **Large Numeric ID Precision** — Preserve large numeric IDs in URL
search params by skipping JSON parsing for plain strings (#3692)
## 🔧 Refactors & Chores
- **Error Propagation for GetAvailable\* APIs** — `GetAvailable*`
methods on `LoggerPlugin`/`LogManager` now return wrapped errors instead
of silently logging and returning empty slices (#3759)
- **Governance Blocklist Matching** — Use `slices.Contains` for VK
blocked-model matching for clearer code with identical semantics (#3727)
- **Exported `ResolvePeriod`** — Renamed `resolvePeriod` to
`ResolvePeriod` so external packages can reuse the period parsing
(#3763)
## 📚 Docs
- **OTEL Env Var Documentation** — Documented `env.VAR_NAME` support for
`collector_url`, `metrics_endpoint`, and headers in OTEL/Prometheus
plugin docs
- **OTEL OSS Features & Examples** — Added OTEL documentation to the OSS
features list with usage examples (#3731)
- **Anthropic Auth Recommendation** — Recommend `ANTHROPIC_AUTH_TOKEN`
over `ANTHROPIC_CUSTOM_HEADERS` for Claude Code authentication (#3686)
2 tasks
akshaydeo
added a commit
that referenced
this pull request
May 27, 2026
## Summary
This PR releases Bifrost OSS `v1.5.5` and Enterprise `v1.4.4`, bumping all module pins from `v1.5.12`/`v1.3.12` to `v1.5.13`/`v1.3.13` across core, framework, and all plugins. It also hardens the Docker manifest shell scripts, expands CI egress allowlists, and updates documentation to reflect the new SCIM-based user provisioning feature.
## Changes
- **Module version bumps**: All `go.mod`/`go.sum` files updated from `core v1.5.12` → `v1.5.13`, `framework v1.3.12` → `v1.3.13`, and all plugin versions incremented accordingly (`compat`, `governance`, `jsonparser`, `logging`, `maxim`, `mocker`, `otel`, `prompts`, `semanticcache`, `telemetry`).
- **Docker manifest scripts**: Added `#!/usr/bin/env bash` shebang and `set -euo pipefail` to `create-docker-manifest.sh` and `create-docker-manifest-ubi9.sh`; quoted all variable expansions and switched `jq -r` to `jq -er` to fail on null digests.
- **CI egress allowlist**: Added `production.cloudfront.docker.com:443` to Docker-related job allowlists, and added `_https._tcp.dl.google.com:443` and `motd.ubuntu.com:443` to the Ubuntu package job allowlist.
- **Changelog files**: Cleared per-module `changelog.md` files (content moved into the new versioned docs). Added `docs/changelogs/v1.5.5.mdx` and `docs/changelogs/ent-v1.4.4.mdx` with full release notes, and registered both in `docs/docs.json`.
- **Documentation**: Replaced the SSO Integration link with a User Provisioning (SCIM) link in both `README.md` and `transports/README.md`.
- **Enterprise v1.4.4 highlights** (documented): Kafka and Google Cloud Pub/Sub observability sinks, chunked streaming with a 100 MB inter-node message ceiling, BigQuery custom labels via env vars using the new `ConfigMarshallerPlugin` interface, temporary access token expiry extensions, and a multi-node cluster integration harness.
- **OSS v1.5.5 highlights** (documented): Azure v1 API migration, env-var support for OTel/Prometheus configs, OTel extra-header forwarding and semantic-convention alignment, virtual key quota including provider configs, Responses API streaming in `jsonparser`, and a batch of Bedrock, Gemini, Anthropic, Azure, and logging plugin fixes.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [x] Documentation
- [x] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [x] Docs
## How to test
```sh
# Core/Transports
go version
go test ./...
# Verify Docker manifest scripts exit on error
bash -n .github/workflows/scripts/create-docker-manifest.sh
bash -n .github/workflows/scripts/create-docker-manifest-ubi9.sh
```
Validate that the new changelog pages (`changelogs/v1.5.5` and `changelogs/ent-v1.4.4`) render correctly in the docs site.
## Screenshots/Recordings
N/A
## Breaking changes
- [x] Yes
- [ ] No
The Azure provider no longer accepts `api_version` in `AzureKeyConfig` and has migrated to the `/openai/v1/{operation}` URL pattern. See the [v1.4.0 Migration Guide](https://docs.getbifrost.ai/enterprise/migration-guides/v1.4.0) for full details.
## Related issues
#3661, #3756, #3651, #3730, #3732, #3754, #3747, #3690, #3729, #3685, #3733, #3761, #3735, #3721, #3720, #3749, #3698, #3762, #3750, #3727, #3717, #3759, #3758, #3764, #3691, #3692, #3737, #3763
## Security considerations
- The `ConfigMarshallerPlugin` interface redacts secrets (OTel collector URLs, Prometheus push gateway credentials, BigQuery labels) at config storage time and rehydrates them at load time, preventing plaintext secret persistence.
- Docker manifest scripts now use `set -euo pipefail`, preventing silent failures that could result in malformed or missing image manifests being pushed.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
This was referenced Jun 18, 2026
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
3 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
Some Bedrock models (e.g. GLM, Llama) do not support prompt-caching cache points in the Converse API and will return a 400 error if cache point blocks are present in the request. This PR adds a guard that strips cache points from Bedrock requests before they are sent, for any model that does not support them.
Changes
BedrockModelSupportsCachePointsincore/schemas/utils.gothat returnstrueonly for Anthropic and Nova models, which are the models known to support explicit cache points in the Converse API.stripCachePointsFromBedrockRequestincore/providers/bedrock/utils.gothat removes cache point blocks from message content, nested tool result content, system messages, and tool config entries.stripCachePointsFromBedrockRequestin bothToBedrockChatCompletionRequestandToBedrockResponsesRequestwhen the target model does not support cache points, preventing 400 errors from unsupported models receiving cache point blocks.Type of change
Affected areas
How to test
Send a Bedrock Converse request to a non-Anthropic, non-Nova model (e.g. a GLM or Llama model) with cache point blocks included in the request body. Verify the request succeeds without a 400 error and that cache point blocks are absent from the forwarded request.
go test ./...Breaking changes
Security considerations
No security implications. Cache point blocks are stripped only from the outbound request payload for unsupported models and do not affect authentication, secrets, or PII handling.
Checklist
docs/contributing/README.mdand followed the guidelines