[fix]: Bedrock provider - ConverseStream egress reports tool_use stop reason for tool-call turns - #5464
Closed
AdityaPainuli wants to merge 31 commits into
Closed
[fix]: Bedrock provider - ConverseStream egress reports tool_use stop reason for tool-call turns#5464AdityaPainuli wants to merge 31 commits into
AdityaPainuli wants to merge 31 commits into
Conversation
…ebhook docs and OpenAPI spec (maximhq#5429) ## Summary Clarifies that webhook delivery for async jobs is opt-in per request, not automatic. Previously, the docs implied that registering an endpoint was sufficient for delivery to occur. This PR corrects that by documenting the `x-bf-async-webhook` header as the explicit trigger, and refines the behavior around subscription validation timing. ## Changes - Updated the async inference tip and webhook overview to state that the endpoint must be named via `x-bf-async-webhook` on the submit request for delivery to occur. - Added a new "Webhook Notifications" section to `async-inference.mdx` detailing opt-in behavior, validation rules, and header scope. - Added a new "Triggering a Delivery" section to `webhooks.mdx` with a curl example and clarifying bullet points. - Corrected the OpenAPI description for `x-bf-async-webhook` to reflect that subscription validation happens at job completion time, not at submission — meaning a missing subscription no longer causes the submit to be rejected, but silently skips delivery instead. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Review the rendered documentation to confirm: 1. The async inference page includes the "Webhook Notifications" section with accurate opt-in behavior. 2. The webhooks page includes the "Triggering a Delivery" section with a working curl example. 3. The OpenAPI spec correctly reflects that subscription absence at job completion skips delivery rather than rejecting the submit. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. No changes to auth, secrets, or delivery signing behavior. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…ent-level overrides (maximhq#5455) ## Summary Replaces the binary on/off switches for deployment-level boolean overrides (Replicate's "use deployments endpoint" and the "use Anthropic endpoints" toggle for SGLang, Deepseek, Fireworks, and vLLM) with a three-way select control. Previously, a plain switch could not distinguish between "explicitly off" and "inherit from the key-level setting," meaning turning the switch off was indistinguishable from leaving it unset. The new `TriStateOverrideRow` component expresses three states: `undefined` (inherit the key's setting), `true` (explicitly on), and `false` (explicitly off). ## Changes - Added a `TriStateOverrideRow` component that renders a select with "Use key setting", "On", and "Off" options, mapping to `undefined`, `true`, and `false` respectively. - Replaced the `Switch`-based inline rows in `ReplicateSection` and `UseAnthropicEndpointsToggleSection` with `TriStateOverrideRow`, preserving the `onChange` contract but now passing the value through directly rather than coercing `false` to `undefined`. - Fixed inconsistent indentation (spaces vs. tabs) in `apiKeysFormFragment.tsx` and `deploymentsTable.tsx`. - Reformatted a few long JSX attribute lists and inline strings for readability. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test 1. Open a provider that supports deployment-level overrides (e.g., Replicate, SGLang, Deepseek, Fireworks, vLLM). 2. Add or edit a deployment and locate the relevant override row. 3. Verify the control renders as a three-option select ("Use key setting", "On", "Off") rather than a toggle switch. 4. Set the value to "Off" and save. Confirm the deployment stores an explicit `false` rather than `undefined`. 5. Set the value to "Use key setting" and save. Confirm the field is stored as `undefined`/absent. ```sh cd ui pnpm i || npm i pnpm build || npm run build ``` ## Screenshots/Recordings Before: A binary switch that could not express "explicitly off" — toggling it off was equivalent to leaving it unset. After: A three-way select with "Use key setting" / "On" / "Off", allowing deployments to explicitly disable a toggle that is enabled at the key level. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## 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
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
…er background colors (maximhq#5451) ## Summary Fixes two issues with the trial expiry banner: the background colours were using Tailwind opacity-modifier classes that didn't render correctly, and the trial expiry date parser rejected RFC3339 timestamps (e.g. `2024-06-01T00:00:00Z`) injected by the Docker build, causing the banner to silently disappear. ## Changes - Replaced `bg-red-500/10` and `bg-amber-500/10` with explicit hex values (`#ffebea` and `#fff4e4`) to ensure the banner background renders as intended in both expired/critical and warning states. - Updated `parseTrialExpiry` to accept both bare `YYYY-MM-DD` dates and full RFC3339 timestamps. Only the calendar date portion is used (interpreted at local midnight), so the time component is stripped before parsing. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test 1. Set `TRIAL_EXPIRY_DATE` to an RFC3339 value such as `2024-06-01T00:00:00Z` and confirm the banner appears with the correct background colour. 2. Set it to a bare date such as `2024-06-01` and confirm the banner still renders correctly. 3. Set it to a date within the warning window and confirm the amber (`#fff4e4`) background is shown. 4. Set it to an expired or critical date and confirm the red (`#ffebea`) background is shown. ```sh cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` ## Screenshots/Recordings Before: banner background was invisible due to unresolved Tailwind opacity-modifier classes. After: banner displays the correct solid tinted background in both warning and expired/critical states. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## 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
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBedrock Converse responses now derive terminal stop reasons from explicit reasons, incomplete details, function-call output, or a default. Replicate tests cover image input conversion and unsupported file URLs. ChangesBedrock Converse stream stop reasons
Replicate image conversion tests
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesStream
participant ToBedrockConverseStreamResponse
participant MessageStop
ResponsesStream->>ToBedrockConverseStreamResponse: completed response metadata
ToBedrockConverseStreamResponse->>ToBedrockConverseStreamResponse: map stop or incomplete reason
ToBedrockConverseStreamResponse->>MessageStop: emit mapped stopReason
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
🤖 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/responses.go`:
- Around line 1799-1800: Update the incomplete-response handling around
bifrostResp.Response.IncompleteDetails in the Bedrock response conversion to map
the reason "max_output_tokens" to Bedrock’s required "max_tokens" value before
assigning stopReason. Preserve existing mappings and behavior for other
incomplete reasons.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05261f96-37a9-4b06-afea-3d54cbaa58c9
📒 Files selected for processing (2)
core/providers/bedrock/conversestreamstopreason_test.gocore/providers/bedrock/responses.go
## Summary `ToReplicateImageGenerationInput` previously ignored `InputImages` entirely and could not surface validation errors to callers. This PR adds input image support to the image generation path (mirroring what already existed for image edits) and propagates URL sanitization errors instead of silently dropping them. ## Changes - Changed `ToReplicateImageGenerationInput` to return `(*ReplicatePredictionRequest, error)` so URL validation errors can be surfaced to callers. - Added `InputImages` handling in the generation path: each image URL is sanitized via `schemas.SanitizeImageURL`, and the resulting slice is routed to the correct model-specific field using the new shared helper. - Extracted the model-to-field dispatch logic (`image_prompt`, `input_image`, `image`, `input_images`) into a `setInputImageField` helper, eliminating the duplicated switch block that previously existed only in the edit path. - Updated both `ImageGeneration` and `ImageGenerationStream` call sites to handle the new error return. - Removed stale line-number references from the Replicate provider docs. ## Type of change - [ ] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh go test ./core/providers/replicate/... ``` New test cases cover: - `InputImages_SingleImageField` — verifies that a kontext-pro model receives the first image in `input_image` and no other image fields are set. - `InputImages_ArrayFieldWithBase64Normalization` — verifies that a generic model receives all images in `input_images` and that bare base64 strings are prefixed with the `data:image/png;base64,` URI scheme. - `InputImages_InvalidURL` — verifies that a `file://` URI causes an error return and a `nil` result. ## Breaking changes - [x] Yes - [ ] No `ToReplicateImageGenerationInput` now returns `(*ReplicatePredictionRequest, error)` instead of `*ReplicatePredictionRequest`. Any external callers must be updated to handle the additional return value. ## Related issues ## Security considerations Input images are now validated through `schemas.SanitizeImageURL` before being forwarded to Replicate. This prevents schemes such as `file://` from being passed through to the provider. ## Checklist - [ ] 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) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Integer constraint fields in the Gemini `Schema` type were tagged with `,string` in their JSON struct tags, causing them to serialize as quoted strings (e.g., `"1"`) rather than JSON numbers (e.g., `1`). This broke strict JSON Schema validation upstream when these constraints were passed through `parametersJsonSchema` (issue maximhq#5433). ## Changes - Removed the `,string` option from the JSON struct tags for `MinItems`, `MaxItems`, `MinLength`, `MaxLength`, `MinProperties`, and `MaxProperties` on the `Schema` type, so these fields now marshal as JSON numbers instead of quoted strings. - Added a round-trip test (`TestGenAIToolSchemaConstraintsRoundTripAsNumbers`) that verifies integer constraints survive the genai → Bifrost → Gemini conversion as proper JSON numbers, covering both numeric and quoted input forms. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/providers/gemini/... -run TestGenAIToolSchemaConstraintsRoundTripAsNumbers -v go test ./core/providers/gemini/... ``` The new test sends a Gemini generation request with integer constraints specified both as raw numbers and as quoted strings, then asserts that after the full round-trip the output constraints are `float64` JSON numbers (not strings). ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues Closes maximhq#5433 ## Security considerations None. ## 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
… does no…" (maximhq#5471) This reverts commit 8354ca7.
… header forwarding (maximhq#5476) ## Summary Documents how to inject dynamic, server-side headers into outgoing MCP requests from a `PreMCPHook` plugin, covering use cases that static configuration cannot address (e.g., per-user identity headers, short-lived service tokens, per-request correlation IDs). ## Changes - Added a tip to the header forwarding section in `connecting-to-servers.mdx` clarifying that forwarded headers come from the caller and are untrusted, and pointing readers to the new plugin recipe for server-side injection. - Added a new "Recipe: injecting dynamic headers server-side" section to `writing-go-plugin.mdx` (v1.5.x+) with a full `PreMCPHook` code example that merges plugin-injected headers into `BifrostContextKeyMCPExtraHeaders`, along with notes on the per-client allowlist, transport compatibility (HTTP/SSE only), and why Connect hooks are the wrong place for per-request identity. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [x] Docs ## How to test Review the rendered documentation to confirm: - The tip in `connecting-to-servers.mdx` links correctly to the new recipe anchor in `writing-go-plugin.mdx`. - The code example in the recipe compiles without errors when dropped into a plugin project. - The `<Note>` and key-points list render correctly in the docs site. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The added documentation explicitly calls out that caller-forwarded headers must be treated as untrusted input by upstream servers, and that plugin-injected identity headers (e.g., signed-in user email) are the appropriate mechanism when the caller must not control the value. The per-client `allowed_extra_headers` allowlist is noted as the enforcement boundary for both forwarded and plugin-injected headers. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Introduces a per-request upstream latency accumulator that tracks cumulative time Bifrost spends blocked on provider sockets across every attempt, retry, fallback, MCP tool call, and media fetch. Subtracting this from total wall time gives Bifrost's own processing overhead — a number that was previously impossible to derive accurately. ## Changes - **New `upstreamlatency.go` schema**: Installs an `*atomic.Int64` accumulator on the `BifrostContext` once per request. Uses an atomic pointer so streaming goroutines can keep writing after the request handler returns without touching the context's value map. - **`ResetUpstreamLatency` / `AddUpstreamLatency` / `GetUpstreamLatency`**: Core API for the accumulator. `Reset` is mandatory at request entry because Bifrost reuses a single process-global context for nil-ctx SDK callers; without it the counter would grow unboundedly. - **`DoStreamingRequest` / `DoHTTPRequest` helpers**: Thin wrappers around `fasthttp.Client.Do` and `net/http.Client.Do` that record the call duration as upstream latency. All provider call sites are migrated to these helpers. - **`idleTimeoutReader.Read` instrumentation**: Each blocking read in a streaming response is counted as upstream time, covering the token-generation window that `DoStreamingRequest` (which returns at first byte) cannot see. - **MCP tool call instrumentation**: `executeToolInternal` wraps `CallTool` with the same accumulator, since waiting on an MCP server is upstream time, not Bifrost overhead. - **`FetchAndEncodeURL` instrumentation**: Remote media fetches are counted as upstream, preventing multi-second fetches from appearing as Bifrost overhead. - **`StampUpstreamLatency` / `PopulateUpstreamLatency`**: Write the accumulated total onto the root trace span (`bifrost.upstream.duration_ms`) and onto `BifrostResponseExtraFields.UpstreamLatency` respectively. Both are called via a named-return `defer` in `handleRequest` so they fire even on error paths. - **`Trace.StampOverheadDuration`**: Computes `bifrost.overhead.duration_ms = root_span_duration - upstream_total` on the export snapshot, after the root span has ended. Clamped at zero to absorb clock skew. - **OTel plugin**: Reads `AttrBifrostOverheadDurationMs` from the root span and records it as a new `bifrost_overhead_latency_seconds` histogram with fine-grained sub-millisecond buckets appropriate for processing overhead rather than network latency. - **Prometheus plugin**: Records the same overhead histogram via the `HTTPTransportPreHook`/`HTTPTransportPostHook` window (widest available, matching the OTel root span). Falls back to the `PostLLMHook` window for SDK callers that bypass the transport layer. - **HTTP transport**: Emits `x-bifrost-upstream-latency-ms` response header so proxy callers can derive overhead from their own elapsed time without parsing the response body. - **New trace attributes**: `bifrost.upstream.duration_ms` and `bifrost.overhead.duration_ms` added to the attribute constant set. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` - Make a request through the HTTP transport and verify the `x-bifrost-upstream-latency-ms` response header is present and less than the total elapsed time. - Make a streaming request and confirm the header value grows to reflect the full generation window, not just time-to-first-byte. - Make a request that triggers a fallback and confirm the upstream latency reflects the sum of both attempts. - In OTel/Prometheus dashboards, verify `bifrost_overhead_latency_seconds` appears and that its values are in the sub-millisecond to low-tens-of-milliseconds range for healthy requests. - Confirm `bifrost.upstream.duration_ms` and `bifrost.overhead.duration_ms` appear on root spans in exported traces. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. The upstream latency value is derived from internal timing and contains no secrets or PII. The new response header exposes only a duration in milliseconds. ## 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
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/providers/bedrock/responses.go (1)
2825-2825: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the non-streaming mapping directly.
Line 2825 changes
ToBedrockConverseResponse, but the added tests shown only exerciseToBedrockConverseStreamResponse. Add a direct regression test formax_output_tokensandcontent_filter, or verify equivalent coverage already exists elsewhere. As per coding guidelines, provider behavior changes should have deterministic coverage for each changed path.🤖 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/responses.go` at line 2825, Add deterministic non-streaming regression coverage for the stop-reason mapping in ToBedrockConverseResponse, specifically verifying max_output_tokens and content_filter map to the expected Bedrock stop reasons. Reuse existing test fixtures and conventions, or confirm equivalent direct coverage already exists elsewhere; do not rely only on ToBedrockConverseStreamResponse tests.Source: Coding guidelines
🤖 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/responses.go`:
- Line 2825: Add deterministic non-streaming regression coverage for the
stop-reason mapping in ToBedrockConverseResponse, specifically verifying
max_output_tokens and content_filter map to the expected Bedrock stop reasons.
Reuse existing test fixtures and conventions, or confirm equivalent direct
coverage already exists elsewhere; do not rely only on
ToBedrockConverseStreamResponse tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a484a33-1382-4b4f-bfc7-6aedcfbbb4db
📒 Files selected for processing (3)
core/providers/bedrock/conversestreamstopreason_test.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.go
## Summary
Fixes a bug where plain-text `SecretVar` objects (e.g. `{"value": "..."}` with no `ref`/`type` fields) were not being recognised as `SecretVar`-shaped during redaction restoration. This caused the UI to persist masked values instead of restoring the real stored secrets when saving Kafka SASL credentials or similar connectors that store secrets as plain strings but return them as value-only objects after a redacted GET.
## Changes
- `isSecretVarObject` previously required either `ref`+`type` or `env_var`+`from_env` alongside `value`, which excluded plain-text `SecretVar`s that marshal as `{"value": "..."}` alone (since `ref`/`type` are `omitempty`). The function now accepts any map whose keys are exclusively drawn from the known `SecretVar` field set (`value`, `ref`, `type`, `env_var`, `from_env`), with `value` required to be a string.
- This ensures that value-only objects round-tripped by the UI after a redacted GET are correctly identified and restored from the existing stored value, rather than being passed through with the masked content.
- Objects with a non-redacted value (e.g. username shown in clear) pass through unchanged, and intentional updates (new password, env reference) are not clobbered.
- Tests added for the Kafka SASL credential shape, the `FullyRedacted()` sentinel (`<REDACTED>`), and intentional secret rotation/env-ref switching.
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./transports/bifrost-http/handlers/...
```
The new tests cover:
- Kafka SASL `password` and `ca_cert` restored from stored plain strings when the UI sends back value-only masked objects.
- `FullyRedacted()` sentinel (`<REDACTED>`) correctly triggers restoration.
- Rotated passwords and env-ref switches pass through without being overwritten by the stored value.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
This change affects how redacted secret values are handled during plugin configuration updates. The fix ensures masked values are never persisted in place of real secrets, and that intentional secret rotations or env-ref changes are not silently discarded. No new secret exposure surface is introduced.
## Checklist
- [x] 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)
- [x] I verified the CI pipeline passes locally if applicable
## Summary Adds a server-configured `batch_role_arn` field to the Bedrock key configuration, allowing operators to pin the IAM service role used for Bedrock batch jobs at the server level rather than relying on clients to supply it via `role_arn` in request extra params. When set, the server-side value takes priority over any client-provided `role_arn`. ## Changes - Added `BatchRoleARN *SecretVar` to `BedrockKeyConfig` in `schemas/account.go`, stored under the JSON key `batch_role_arn` and kept separate from the STS AssumeRole identity (`bedrock_role_arn`). - Updated `BatchCreate` in `bedrock.go` so that `key.BedrockKeyConfig.BatchRoleARN` is resolved first; the client-supplied `role_arn` in `ExtraParams` is only used as a fallback when the server value is absent. - Added a database migration (`add_bedrock_batch_role_arn_column`) that adds the `bedrock_batch_role_arn` column to `config_keys`, with rollback support. - Wired `BedrockBatchRoleARN` through all RDB read/write paths (`tableKeyFromSchemaKey`, `UpdateProvidersConfig`, `UpdateProvider`, `AddProvider`) and through `BeforeSave`/`AfterFind` hooks including encryption and decryption. - Updated the `AfterFind` Bedrock config reconstruction condition to include `BedrockBatchRoleARN`. - Added `BatchRoleARN` to the `mergeUpdatedKey` preserve logic in the HTTP handler so partial updates do not accidentally clear the field. - Added `batch_role_arn` to the config JSON schema with a description noting its priority semantics and `env.` prefix support. - Added `batch_role_arn` to the Zod schemas (`providerForm.ts`, `schemas.ts`) and rendered a **Batch Role ARN** input field in the UI form, visible only when the provider supports the batch API. - Added redaction support for `BatchRoleARN` in `clientconfig.go`. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` **Manual validation:** 1. Configure a Bedrock provider key with `batch_role_arn` set (either as a literal ARN or via `env.AWS_BATCH_ROLE_ARN`). 2. Submit a batch create request that also includes `role_arn` in `extra_params`. 3. Confirm that the server-configured `batch_role_arn` is used and the client-supplied value is ignored. 4. Remove `batch_role_arn` from the key config and resubmit; confirm the client-supplied `role_arn` is now used. 5. Verify the value is stored encrypted in the database and appears redacted in API responses. **New config field:** | Field | JSON key | Description | |---|---|---| | `BatchRoleARN` | `batch_role_arn` | Service role ARN Bedrock assumes for batch S3 access. Supports `env.` prefix. Takes priority over client-supplied `role_arn`. | ## Breaking changes - [ ] Yes - [x] No ## Security considerations `BatchRoleARN` is treated as a secret: it is encrypted at rest via the existing `encryptSecretVarPtr`/`decryptSecretVarPtr` pipeline and redacted in API responses, consistent with other credential fields such as `RoleARN` and `ExternalID`. ## 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
akshaydeo
dismissed
coderabbitai[bot]’s stale review
July 24, 2026 22:43
The merge-base changed after approval.
|
|
…iew maintenance (maximhq#5693) * feat: support matview_refresh_interval "off" to disable logstore matview maintenance The materialized views back only the dashboard UI. Deployments that run Bifrost headless behind their own observability stack pay the REFRESH MATERIALIZED VIEW CONCURRENTLY cost for views nothing reads, and the 5s floor means the interval alone cannot turn maintenance off. With "off" (or a non-positive duration) the logs store skips view creation, the initial refresh, and the periodic refresher entirely. matViewsReady stays false, so dashboard queries fall back to the raw tables, and the runtime self-heal path cannot re-arm maintenance since it only triggers from matview-path queries. * fix: guard matview self-heal when maintenance is disabled Review follow-up: carry the resolved disabled state onto the store so triggerMatViewSelfHeal cannot recreate views the configuration says must not exist, and make the schema/docs explicit that a zero duration also disables (positive sub-5s values still clamp up).
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved token parameter compatibility handling to preserve alternative formats when the primary option is unsupported. * **Chores** * Version updated to 2.0.0. * Enhanced load testing configuration for more reliable builds. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
## Summary Adds a `THIRD_PARTY_NOTICES.md` file to formally document third-party components used in Bifrost that carry license terms requiring explicit attribution — specifically MPL-2.0 licensed dependencies and embedded source code derived from external projects. ## Changes - Introduces `THIRD_PARTY_NOTICES.md` to attribute: - Embedded source code in `framework/migrator/migrator.go` derived from `go-gormigrate/gormigrate` (MIT) - Go binary dependencies carrying MPL-2.0 terms: `github.com/cyphar/filepath-securejoin` and `github.com/hashicorp/go-version` - npm build-time devDependencies carrying MPL-2.0 terms: `lightningcss` (never shipped to end users) and `dompurify` (Apache-2.0 option elected) - All MPL-2.0 components are used unmodified and combined as a "Larger Work" per MPL-2.0 Section 3.3; no Bifrost source files are themselves MPL-licensed. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test No functional changes — review the file contents to confirm accuracy of license attributions against the listed upstream repositories. ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations This change has no security implications. It is a legal/compliance attribution document only. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
## Summary Bumps several Go dependencies to their latest patch/minor versions across all modules in the repository. ## Changes - `github.com/aws/aws-sdk-go-v2/service/s3`: `v1.97.3` → `v1.99.0` - `github.com/aws/aws-sdk-go-v2/config`: `v1.32.11` → `v1.32.14` - `github.com/aws/aws-sdk-go-v2/internal/ini`: `v1.8.5` → `v1.8.6` - `github.com/weaviate/weaviate`: `v1.36.5` → `v1.38.0` - `github.com/buger/jsonparser`: `v1.1.2` → `v1.2.0` - `github.com/go-openapi/spec`: `v0.22.2` → `v0.22.3` - `github.com/google/cel-go`: `v0.28.1` → `v0.29.0` - `github.com/stretchr/objx`: `v0.5.3` added as an indirect dependency ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. All changes are dependency version bumps with no security-sensitive modifications. ## 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
…aximhq#5759) ## Summary Closes a race-condition security gap where an unauthenticated network caller could reach a freshly deployed, not-yet-configured Bifrost instance and create the first admin account before the real operator does. Previously, `PUT /api/config` was intentionally open when no admin account existed (zero-config UX), but this left a window of exposure on any publicly reachable host. The fix introduces a one-time **setup token** — generated in-memory at startup when no admin account is configured, printed to the server's startup logs, and required alongside the username/password when creating the first admin account. The token is never persisted, is regenerated on every restart until an admin account exists, and is permanently invalidated once the first admin account is created. ## Changes - **Bootstrap token generation (`middlewares.go`):** `InitAuthMiddleware` generates a UUID setup token via `atomic.Pointer[string]` when no admin account is configured, logs it prominently to stdout, and exposes `CheckBootstrapToken` (constant-time comparison) and `ClearBootstrapToken` methods. - **Token validation in the config handler (`config.go`):** `updateConfig` now calls `ValidateSetupToken` before allowing the first admin account to be created. Returns HTTP 403 if the token is missing or wrong. - **Token cleared on first admin account creation (`server.go`):** `UpdateAuthConfig` calls `ClearBootstrapToken` after successfully persisting the first admin account, permanently closing the gate. - **`setup_token`** **field added to** **`UpdateConfigRequest`:** The field is accepted in the request body but never persisted or returned by `GET /api/config`. - **UI (`securityView.tsx`):** When no `auth_config` exists server-side (`isFirstTimeSetup`), a **Setup token** input field is shown below the password field. The token is validated client-side before submission and cleared from state after a successful save. - **TypeScript types (`config.ts`):** `setup_token?: string` added to `BifrostConfig`. - **OpenAPI schema (`config.yaml`):** `setup_token` documented on `UpdateConfigRequest`. - **Docs:** A `<Warning>` block added to `security-best-practices.mdx` and a `<Note>` added to `setting-up-auth.mdx` explaining the setup token flow, where to find it, and that it only applies once. - **Tests (`middlewares_test.go`):** Two new test cases cover the no-token-generated (pass-through) case and the validate-then-clear lifecycle. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [x] Docs ## How to test **Manual flow:** 1. Start a fresh Bifrost instance with no existing admin account. 2. Check startup logs for the block beginning `No admin account is configured for this Bifrost instance yet.` and copy the setup token. 3. Open the dashboard → Security Settings. Confirm the **Setup token** field appears below the password field. 4. Attempt to save with auth enabled but without the setup token — expect a toast error. 5. Paste the correct token and save — expect success and the Setup token field to disappear on reload. 6. Confirm that `PUT /api/config` without the token returns HTTP 403 while no admin account exists. 7. Restart the server before completing setup and confirm a new token is printed. ```sh # Core/Transports go test ./transports/bifrost-http/handlers/... # UI cd ui pnpm i pnpm build ``` ## Breaking changes - [x] Yes - [ ] No Any automation or scripts that call `PUT /api/config` to create the first admin account on a fresh instance must now include `setup_token` in the request body. The token is available in the server's startup logs. Instances that already have an admin account configured are unaffected — the field is ignored once an admin account exists. ## Security considerations - The setup token is generated with `uuid.NewString()` (crypto-random UUID), stored only in process memory, and compared with `crypto/subtle.ConstantTimeCompare` to prevent timing attacks. - The token is never written to disk, never returned by any API endpoint, and is permanently invalidated after first use. - Operators must have access to the process's stdout/log stream (`docker logs`, `kubectl logs`, or terminal) to retrieve the token, which is the same access level required to operate the host — this is the intended trust boundary. ## 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) - [ ] I verified the CI pipeline passes locally if applicablecg
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
## Summary Loading a custom plugin `path` causes native code (a `.so`) to be `dlopen()`'d directly into the gateway process. Previously, this was allowed even when dashboard authentication was disabled or unconfigured — meaning any caller who could reach the management API could inject arbitrary native code. This PR closes that gap by requiring a genuinely authenticated admin session for any create or update operation that sets a non-builtin plugin `path`, and separately hardens the plugin downloader against SSRF. ## Changes - Added `BifrostContextKeyAuthBypassed` context key, set by the auth middleware exclusively when a request is let through because dashboard auth is disabled/unconfigured (distinct from `IsLocalAdminContextKey`, which is also set on real authenticated sessions). - `createPlugin` and `updatePlugin` handlers now check `BifrostContextKeyAuthBypassed` and return `403` before any DB write when a non-builtin `path` is supplied without genuine authentication. - Replaced the `fasthttp`-based plugin downloader with a `net/http` client backed by `network.SSRFSafeDialContext`, matching the SSRF hardening already applied to `core/providers/utils.FetchAndEncodeURL`. The new client: rejects non-`http`/`https` schemes before any network call, refuses connections to loopback, private, CGNAT, link-local, and unspecified addresses (including IPv4-in-IPv6 transition addresses) at dial time (not just DNS lookup time, so DNS rebinding doesn't bypass it), applies the same IP check to redirect targets, caps redirect depth at 5, and limits response body reads to 200 MB. - Tests for `DownloadPlugin` now use a `useNonSSRFGuardedClient` helper that swaps in a plain dialer for the duration of each test (since `httptest` servers bind to loopback, which the production dialer correctly blocks). A new `TestDownloadPlugin_BlocksSSRFToLoopback` test verifies the production guard is active by default, and `TestDownloadPlugin_RejectsNonHTTPScheme` verifies `file://` and similar schemes are rejected before any network call. - New handler tests cover all four cases: create with bypassed auth (expect 403, no DB write), create with real auth (expect 201, path stored), update with bypassed auth (expect 403, no DB write), and the existing config-merge behaviour. - OpenAPI docs and the plugin sequencing guide updated to document the 403 response and the authentication requirement for `path`. - Dependency bumps: `aws-sdk-go-v2/config` → v1.32.14, `aws-sdk-go-v2/service/s3` → v1.99.0, `aws-sdk-go-v2/internal/ini` → v1.8.6, `buger/jsonparser` → v1.2.0. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI > This is primarily a security hardening change. ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh # Run all tests go test ./... # Specifically verify the new plugin handler guards go test ./transports/bifrost-http/handlers/... -run TestCreatePlugin_RejectsCustomPathWhenAuthBypassed go test ./transports/bifrost-http/handlers/... -run TestCreatePlugin_AllowsCustomPathWhenNotBypassed go test ./transports/bifrost-http/handlers/... -run TestUpdatePlugin_RejectsCustomPathWhenAuthBypassed # Verify SSRF guard on plugin downloader go test ./framework/plugins/... -run TestDownloadPlugin_BlocksSSRFToLoopback go test ./framework/plugins/... -run TestDownloadPlugin_RejectsNonHTTPScheme ``` To manually verify the 403 behaviour: start the gateway with no dashboard auth configured, then attempt `POST /api/plugins` with a `path` field pointing to a `.so`. The response should be `403` with a message instructing the operator to enable dashboard authentication first. ## Breaking changes - [x] Yes - [ ] No Operators running with dashboard authentication disabled who were previously able to create or update custom plugin paths via the API will now receive a `403`. To restore the capability, enable dashboard authentication and authenticate before calling those endpoints. ## Security considerations - Closes an unauthenticated native code injection vector: without this change, any network-reachable caller could `dlopen()` an attacker-controlled `.so` into the gateway process when dashboard auth was off. - The SSRF fix on the plugin downloader prevents a crafted plugin URL from causing the gateway to fetch from internal/metadata endpoints (e.g. cloud IMDS). The guard runs at dial time, not DNS resolution time, so DNS rebinding attacks do not bypass it. - `BifrostContextKeyAuthBypassed` is intentionally separate from `IsLocalAdminContextKey` so that future handlers gating other high-risk operations can use the same signal without ambiguity. ## 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
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes maximhq#123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## 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
…preason-5206 # Conflicts: # core/schemas/bifrost.go # core/schemas/context.go # docs/deployment-guides/config-json/guardrails.mdx # docs/deployment-guides/helm/guardrails.mdx # docs/docs.json # docs/enterprise/guardrails.mdx # framework/configstore/migrations.go # framework/logstore/matviews.go # framework/logstore/postgres.go # framework/tracing/tracer.go # plugins/otel/main.go # transports/bifrost-http/integrations/utils.go
2 tasks
akshaydeo
dismissed
coderabbitai[bot]’s stale review
August 5, 2026 07:18
The merge-base changed after approval.
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
9 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
On the Bedrock ConverseStream egress, a turn where the model calls a tool streams the terminal
messageStopevent withstopReason: "end_turn"instead of"tool_use". Truncated turns similarly reportend_turninstead ofmax_tokens. Clients that branch on the stop reason (agent loops deciding whether to execute tools and continue) misread the turn as finished.Changes
core/providers/bedrock/responses.go: theresponse.completedhandler inToBedrockConverseStreamResponsehardcodedend_turnand only overrode it fromIncompleteDetails, ignoring theStopReasonandOutputthe completed event already carries. It now mirrors the derivation chain the non-streamingConverseconverter already uses:Response.StopReason(via the existingconvertBifrostToBedrockStopReasonmap) →IncompleteDetails→ tool-use detection fromResponse.Output→end_turnfallback.core/providers/bedrock/conversestreamstopreason_test.go(new): replays full stream lifecycles through the converter and asserts themessageStoppayload.No new mappings introduced; the fix reuses the existing stop-reason map and matches the non-streaming path exactly.
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
Expected: all TestConverseStream* tests pass. On dev without the fix, TestConverseStreamToolUseStopReason, TestConverseStreamToolUseStopReasonFromOutput, and TestConverseStreamLengthStopReason fail with stopReason: "end_turn".
Cases covered:
If adding new configs or environment variables, document them here.
Screenshots/Recordings
N/A (no UI changes).
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Closes #5206
Security considerations
None. Output-side stop-reason mapping only, no auth/secrets/PII involved.
Checklist
docs/contributing/README.mdand followed the guidelines