fix: add graceful fallback for unsupported count_tokens - #3673
fix: add graceful fallback for unsupported count_tokens#36730xPixelNinja wants to merge 93 commits into
Conversation
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an opt-in ChangesCount Tokens Graceful Fallback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as Bifrost HTTP
participant PreLLMHook
participant Provider
participant PostLLMHook
Client->>HTTP: CountTokensRequest with optional x-bf-compat
HTTP->>PreLLMHook: Request and context
PreLLMHook->>Provider: Forward request
Provider->>PostLLMHook: unsupported_operation error
PostLLMHook->>PostLLMHook: Estimate tokens and build response
PostLLMHook->>Client: CountTokensResponse
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/count_tokens_fallback.go (1)
137-143: ⚡ Quick winHonor
ResponsesMessageContentsource exclusivity in token estimation.When
ContentStris non-nil,ContentBlocksshould not be counted in the same message. Using both can over-estimate tokens and diverges from the schema contract.♻️ Proposed fix
if msg.Content != nil { if msg.Content.ContentStr != nil { estimate = estimate.withText(estimateTokensFromText(*msg.Content.ContentStr)) - } - for _, block := range msg.Content.ContentBlocks { - estimate = estimate.add(estimateCountTokensFromContentBlock(block)) + } else { + for _, block := range msg.Content.ContentBlocks { + estimate = estimate.add(estimateCountTokensFromContentBlock(block)) + } } }Based on learnings:
schemas.ResponsesMessageContenttreatsContentStrandContentBlocksas mutually exclusive; only useContentBlockswhenContentStris nil.🤖 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/count_tokens_fallback.go` around lines 137 - 143, The token estimator is double-counting when both ResponsesMessageContent.ContentStr and ContentBlocks are present; update the logic in core/count_tokens_fallback.go (the branch that inspects msg.Content) to treat ResponsesMessageContent as exclusive: if msg.Content.ContentStr != nil, only call estimate.withText(estimateTokensFromText(*msg.Content.ContentStr)) and skip iterating ContentBlocks; otherwise (ContentStr == nil) iterate msg.Content.ContentBlocks and add estimate.add(estimateCountTokensFromContentBlock(block)) as before so ContentBlocks are only counted when ContentStr is nil.
🤖 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 `@tests/integrations/python/config.yml`:
- Line 324: provider_scenarios.azure has count_tokens enabled but
providers.azure lacks a corresponding model mapping; add a count_tokens entry
under the providers.azure models mapping so the scenario resolves to a concrete
Azure model. Locate the providers.azure configuration block and add a key named
count_tokens mapped to the appropriate Azure model identifier (the same model
type used for token counting elsewhere in the repo or tests), ensuring the
mapping name matches provider_scenarios.azure.count_tokens so runtime resolution
will succeed.
---
Nitpick comments:
In `@core/count_tokens_fallback.go`:
- Around line 137-143: The token estimator is double-counting when both
ResponsesMessageContent.ContentStr and ContentBlocks are present; update the
logic in core/count_tokens_fallback.go (the branch that inspects msg.Content) to
treat ResponsesMessageContent as exclusive: if msg.Content.ContentStr != nil,
only call estimate.withText(estimateTokensFromText(*msg.Content.ContentStr)) and
skip iterating ContentBlocks; otherwise (ContentStr == nil) iterate
msg.Content.ContentBlocks and add
estimate.add(estimateCountTokensFromContentBlock(block)) as before so
ContentBlocks are only counted when ContentStr is nil.
🪄 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: 13d5a499-8b38-44bb-9b66-b8f77b81385a
📒 Files selected for processing (4)
core/bifrost.gocore/bifrost_test.gocore/count_tokens_fallback.gotests/integrations/python/config.yml
Confidence Score: 5/5Safe to merge — the fallback is strictly opt-in, the estimation path is isolated to a new file, and the existing error-passthrough behavior is fully preserved when the feature is disabled. All changed paths are additive: a new DB column with a safe false default, a new estimation function that only runs when explicitly enabled, and hooks that short-circuit immediately when the state key is absent. The DB migration is idempotent, the fallback only fires after all configured provider fallbacks are exhausted, and the test suite covers the four critical behavioral contracts. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix: move count_tokens fallback to compa..." | Re-trigger Greptile |
3f4c861 to
2d5f76e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/count_tokens_fallback.go (1)
1-1: ⚡ Quick winRename this file to match the Go filename convention.
core/count_tokens_fallback.gouses underscores; the repository rule for non-test Go files requires concatenated lowercase words.As per coding guidelines, "No underscores in Go filenames except for _test.go suffix; concatenate words in lowercase for multi-word filenames."
🤖 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/count_tokens_fallback.go` at line 1, The file name uses underscores which violates the repo Go filename convention; rename core/count_tokens_fallback.go to a concatenated lowercase name like core/counttokensfallback.go and update any references or build/import paths if the filename is referenced elsewhere (ensure package bifrost and the function/identifier names such as CountTokensFallback remain unchanged); run go build or go test to verify the package compiles after the rename.
🤖 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/count_tokens_fallback.go`:
- Line 1: The file name uses underscores which violates the repo Go filename
convention; rename core/count_tokens_fallback.go to a concatenated lowercase
name like core/counttokensfallback.go and update any references or build/import
paths if the filename is referenced elsewhere (ensure package bifrost and the
function/identifier names such as CountTokensFallback remain unchanged); run go
build or go test to verify the package compiles after the rename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d961abf-cee5-4c3f-b826-048f88a97777
📒 Files selected for processing (4)
core/bifrost.gocore/bifrost_test.gocore/count_tokens_fallback.gotests/integrations/python/config.yml
2d5f76e to
cb1c9aa
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integrations/python/config.yml (1)
324-324:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAzure
count_tokensenablement is incomplete without a provider model mapping.
provider_scenarios.azure.count_tokensis now enabled, butproviders.azurestill has nocount_tokensentry. This can break capability resolution or route incorrectly at runtime.Suggested patch
azure: chat: "gpt-4o" vision: "gpt-4o" tools: "gpt-4o-mini" streaming: "gpt-4o-mini" speech: "gpt-4o-mini-tts" transcription: "whisper" embeddings: "text-embedding-3-small" image_generation: "gpt-image-1" thinking: "o1" + count_tokens: "gpt-4o-mini" batch_file_upload: "gpt-4o-2"🤖 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 `@tests/integrations/python/config.yml` at line 324, provider_scenarios.azure.count_tokens was enabled but providers.azure lacks a corresponding count_tokens mapping; update the configuration by adding a count_tokens entry under providers.azure that maps the Azure model(s) used for token counting (matching the keys referenced by provider_scenarios.azure) so capability resolution can find the provider implementation (look for provider_scenarios.azure.count_tokens and providers.azure to add the appropriate count_tokens mapping).
🤖 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.
Duplicate comments:
In `@tests/integrations/python/config.yml`:
- Line 324: provider_scenarios.azure.count_tokens was enabled but
providers.azure lacks a corresponding count_tokens mapping; update the
configuration by adding a count_tokens entry under providers.azure that maps the
Azure model(s) used for token counting (matching the keys referenced by
provider_scenarios.azure) so capability resolution can find the provider
implementation (look for provider_scenarios.azure.count_tokens and
providers.azure to add the appropriate count_tokens mapping).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bc1b251-4163-489d-bdb5-704cafc2e9ae
📒 Files selected for processing (4)
core/bifrost.gocore/bifrost_test.gocore/counttokensfallback.gotests/integrations/python/config.yml
|
Pushed the cleanup and follow up fixes, please merge if this looks good |
The merge-base changed after approval.
|
Hey @0xPixelNinja thanks for the PR! This feature is useful but I think we should move it to compat plugin under a toggle cause not everyone would want this behavior |
Sure, I will move it to the compat plugin behind a toggle :) |
cb1c9aa to
a99ce71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/compat/counttokensfallback_test.go (1)
30-38: ⚡ Quick winAdd explicit tests for disabled config and request-level override.
Line 33 hard-codes
CountTokensFallback: true, so this suite never validates the off-by-default path or request override path in this stack. Please add one test withCountTokensFallback: false(expect originalunsupported_operationto remain) and one with per-request override enabled (expect synthesized response).Proposed minimal refactor to enable both branches in tests
-func newCompatPluginForCountTokensFallback(t *testing.T, account schemas.Account) *CompatPlugin { +func newCompatPluginForCountTokensFallback(t *testing.T, account schemas.Account, enabled bool) *CompatPlugin { t.Helper() - plugin, err := Init(Config{CountTokensFallback: true}, bifrost.NewNoOpLogger(), nil, account) + plugin, err := Init(Config{CountTokensFallback: enabled}, bifrost.NewNoOpLogger(), nil, account) if err != nil { t.Fatalf("init compat plugin: %v", err) } return plugin }- plugin := newCompatPluginForCountTokensFallback(t, nil) + plugin := newCompatPluginForCountTokensFallback(t, nil, true)🤖 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 `@plugins/compat/counttokensfallback_test.go` around lines 30 - 38, The test helper newCompatPluginForCountTokensFallback always sets Config.CountTokensFallback=true so tests never exercise the disabled-default and request-level override paths; update tests by (A) adding a test that constructs the CompatPlugin via Init with Config{CountTokensFallback:false} and asserts the original unsupported_operation behavior remains, and (B) adding a test that constructs the plugin with CountTokensFallback:false but sends a request with the per-request override enabled (use whatever request field/flag the code inspects for request-level fallback) and asserts the synthesized response is returned; to implement this you can either add a new helper that accepts a countTokensFallback bool or overload newCompatPluginForCountTokensFallback to accept that flag, and locate initialization logic in Init/Config and request handling code that checks the per-request override to craft the assertions.
🤖 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 `@plugins/compat/counttokensfallback.go`:
- Around line 170-172: The current conditional only counts custom tool input
when both msg.ResponsesToolMessage and msg.ResponsesCustomToolCall are non-nil,
which can undercount; change the check to simply if msg.ResponsesCustomToolCall
!= nil { estimate =
estimate.withText(estimateTokensFromText(msg.ResponsesCustomToolCall.Input)) }
so that estimateTokensFromText is invoked whenever a ResponsesCustomToolCall
exists (retain the existing variables: msg.ResponsesCustomToolCall, estimate,
estimate.withText, and estimateTokensFromText).
---
Nitpick comments:
In `@plugins/compat/counttokensfallback_test.go`:
- Around line 30-38: The test helper newCompatPluginForCountTokensFallback
always sets Config.CountTokensFallback=true so tests never exercise the
disabled-default and request-level override paths; update tests by (A) adding a
test that constructs the CompatPlugin via Init with
Config{CountTokensFallback:false} and asserts the original unsupported_operation
behavior remains, and (B) adding a test that constructs the plugin with
CountTokensFallback:false but sends a request with the per-request override
enabled (use whatever request field/flag the code inspects for request-level
fallback) and asserts the synthesized response is returned; to implement this
you can either add a new helper that accepts a countTokensFallback bool or
overload newCompatPluginForCountTokensFallback to accept that flag, and locate
initialization logic in Init/Config and request handling code that checks the
per-request override to craft the assertions.
🪄 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: 2eb47717-cded-47b2-84ec-bd01c635c2cf
📒 Files selected for processing (12)
core/schemas/bifrost.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goplugins/compat/counttokensfallback.goplugins/compat/counttokensfallback_test.goplugins/compat/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/plugins.gotransports/config.schema.json
✅ Files skipped from review due to trivial changes (1)
- core/schemas/bifrost.go
a99ce71 to
fcece06
Compare
|
@Pratham-Mishra04, moved it to the compat plugin please review when you get a chance |
The merge-base changed after approval.
…re-completion 409 on complete-oauth
* fix(ui): skip password validation for redacted credential * fix(ui): validate newly entered redaction sentinels
…aggregates (maximhq#5737) ## Summary Adds a `roots_only` filter to the log search API that collapses fallback chains into a single root row. When enabled, any log whose `parent_request_id` points at an actual log row is hidden from the list view, leaving only the chain's root visible. Each root is annotated with child aggregates (`child_count`, `children_cost`, `children_tokens`) so the UI can render an expandable row summarizing the full chain without additional queries. ## Changes - Added `RootsOnly bool` to `SearchFilters` and wired it to the `roots_only` query parameter in the HTTP handler via `strconv.ParseBool`. - In `applyFilters`, when `RootsOnly` is set and no `ParentRequestID` filter is active, a subquery filters out rows whose `parent_request_id` matches an existing log ID. ClickHouse uses an uncorrelated `NOT IN` subquery (correlated subqueries are unsupported); all other dialects use `NOT EXISTS`. - After a `roots_only` search returns a page, `attachChildAggregates` runs a single grouped query over the page's IDs to populate `ChildCount`, `ChildrenCost`, and `ChildrenTokens` on each root. These fields are transient (`gorm:"-"`) and never stored. - `ParentRequestID` takes precedence over `RootsOnly` — when a parent filter is active the full child list is returned, matching the expand-on-click behaviour. - `canUseMatViewFilters` now returns `false` when `RootsOnly` is set, since the per-row existence predicate cannot be expressed in the hourly materialized view count path. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/logstore/... -run TestSearchLogsRootsOnly go test ./framework/logstore/... -run TestCanUseMatViewFiltersExcludesRootsOnly go test ./... ``` **HTTP:** ```sh GET /logs?roots_only=true ``` Expected: only root rows returned, each carrying `child_count`, `children_cost`, and `children_tokens` where children exist. ```sh GET /logs?roots_only=true&parent_request_id=<id> ``` Expected: `roots_only` is ignored; all children of the given parent are returned. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The `roots_only` subquery operates only on the `logs` table within the tenant-scoped DB connection. No new data is exposed; child rows remain accessible via `GetSessionLogs` using the root's ID. ## 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) - [ ] I verified the CI pipeline passes locally if applicable
* docs: add Bedrock runbooks for Claude Code and Codex * docs: use Bedrock deployment mappings in runbooks * remove unecessary warning * replace static json with UI image * docs: add Edge setup paths to Bedrock runbooks * docs: clarify Claude Code model validation
## Summary Improves the documentation for Datadog integration configuration fields to clarify that `service_name`, `ml_app`, `env`, and `version` all support the `env.VAR_NAME` prefix for environment variable substitution at runtime. ## Changes - Added descriptions to previously undocumented `service_name`, `env`, and `version` fields in the Helm chart schema, explicitly noting `env.VAR_NAME` substitution support with examples - Updated `ml_app` description in the Helm chart schema to mention `env.VAR_NAME` support - Updated `service_name`, `ml_app`, `env`, and `version` descriptions in the transport config schema to note the `env.` prefix capability - Added inline comments in `values.yaml` for `service_name`, `env`, `version`, and `ml_app` to surface the `env.VAR_NAME` support directly in the default config ## 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 behavioral changes. Validate that the schema descriptions render correctly by inspecting the JSON schema files and confirming the Helm chart lints cleanly. ```sh helm lint helm-charts/bifrost ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. These are documentation-only changes to schema descriptions and YAML comments. ## 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 Adds end-to-end observability for guardrail judge calls — the internal LLM invocations made by the enterprise guardrails plugin to evaluate rules. Previously, these calls were invisible: their token spend was untracked, their outcomes were not logged, and their cost was not reflected in billing. This PR surfaces that data through a new `guardrail_debug` field on responses, log entries, and the UI. ## Changes - Introduced `BifrostGuardrailDebug` and `BifrostGuardrailJudgeCall` schema types in a new `guardraildebug.go` file, with typed context helpers (`GuardrailDebugFromContext`, `SetGuardrailDebugOnContext`, `AppendGuardrailJudgeCallOnContext`) that enforce copy-on-read isolation so callers cannot mutate context state. - Added `BifrostContextKeyGuardrailDebug` context key and `GuardrailDebug *BifrostGuardrailDebug` to `BifrostResponseExtraFields`, propagated through all response conversion paths (`ToTextCompletionResponse`, `ToBifrostTextCompletionResponse`) and all streaming accumulators and chunk types. - Extended `StreamAccumulatorResult` and `AccumulatedData` with `GuardrailDebug` so streaming pipelines carry the field through to the final assembled response. - Added `CalculateGuardrailCost` to the model catalog datasheet and exposed it via `ModelCatalog`. `CalculateCost` now adds judge-call cost on top of the main request cost (including cache-hit paths). Judge cost is attributed to the judge's own provider/model, preserving virtual-key attribution. - Added a `guardrail_debug` column to the logstore `Log` table via a new migration, with full serialize/deserialize, payload extraction, merge, and clear support. - Updated the logging plugin's `PostLLMHook` to read guardrail debug from context (covering input-block cases where no provider response exists) and from the response, write it to the log entry, and apply guardrail cost to `entry.Cost` — including for error paths and streaming. - Updated `calculateCostForLog` to treat a non-nil `guardrailDebug` as sufficient to proceed with cost calculation, so input-blocked requests are billed correctly. - Added `GuardrailDebug` and `GuardrailJudgeCall` TypeScript types and rendered a "Guardrail Details" section in the log detail view showing rule, phase, action (Blocked/Allowed badge), guardrail name and provider, judge provider and model, token counts, and reason. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./core/schemas/... ./framework/logstore/... ./framework/modelcatalog/... ./framework/streaming/... ./plugins/logging/... # UI cd ui pnpm i || npm i pnpm build || npm run build ``` To validate end-to-end: 1. Send a request through a guardrail rule that triggers a judge call. 2. Confirm the response `extra_fields.guardrail_debug.judge_calls` is populated with provider, model, and token counts. 3. Open the log detail view and verify the "Guardrail Details" section appears with correct phase, action badge, and token counts. 4. Confirm `cost` on the log entry reflects both the main request and the judge call spend. 5. For an input-blocked request (no provider response), confirm `guardrail_debug` and cost are still written to the log. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `guardrail_debug` is written to the log store and returned in API responses. It does not contain prompt content — only metadata (rule name, provider, model, token counts, action, reason). The `reason` field may contain guardrail-generated explanations; ensure content logging policies are applied consistently if reason strings are considered sensitive. ## 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) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Clarifies that passthrough endpoints are not credential proxies — Bifrost always selects and injects its own provider key, and any provider credentials supplied by the caller are stripped before the request is forwarded upstream. ## Changes - Added a `Warning` callout making it explicit that callers must authenticate with a Bifrost virtual key, not a provider API key, and that provider keys in the request are never forwarded. - Added a `Note` callout explaining that Claude Code OAuth tokens (`sk-ant-oat…`) are handled on the regular `/anthropic` route, not via passthrough. - Updated the "How it works" numbered steps to explicitly describe Bifrost's key selection and credential-stripping behavior. - Updated curl examples for Anthropic, GenAI (Gemini), and Vertex passthrough to use `<YOUR-BIFROST-VIRTUAL-KEY>` instead of raw provider API key placeholders. - Replaced the Azure-specific auth note in the Notes section with a provider-agnostic statement covering all passthrough endpoints (`authorization`, `api-key`, `x-api-key`, `x-goog-api-key`). - Added a note about the `direct API keys` exception, requiring both `allow_direct_keys` server-side and `x-bf-direct-key: true` per-request. ## 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 updated passthrough documentation and verify: - The `Warning` and `Note` callouts render correctly. - curl examples reference `<YOUR-BIFROST-VIRTUAL-KEY>` consistently across Anthropic, GenAI, and Vertex sections. - The Notes section accurately reflects the behavior for all passthrough endpoints, not just Azure. ## Breaking changes - [ ] Yes - [x] No ## Security considerations This change reinforces that provider API keys should never be sent by callers on passthrough requests — Bifrost strips them regardless. The documentation now makes this behavior explicit, reducing the risk of users inadvertently exposing provider credentials or expecting them to be forwarded upstream. ## 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 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
…OTEL (maximhq#5939) ## Summary Adds a `traces_enabled` flag to OTel profiles, allowing a profile to operate in a metrics-only mode without requiring a `collector_url`. Previously, every enabled profile required a collector URL because traces were always on. This change decouples trace and metrics export so each can be independently toggled. ## Changes - Added `traces_enabled` boolean field to `Profile` with a default of `true` so existing configs continue exporting spans without modification. - `collector_url` is now only required when `traces_enabled` is `true`; a metrics-only profile (`traces_enabled: false`, `metrics_enabled: true`) no longer needs one. - The trace client is only built when `traces_enabled` is `true`; `Inject` already skips a nil client. - Protocol validation is skipped entirely when both traces and metrics are disabled (no-op profile). - The JSON schema's `collector_url` requirement condition was updated to account for `traces_enabled: false`, and `protocol` was added to the `metrics_enabled` requirement. - The `profileForStorage` struct and `MarshalForStorage` now persist `traces_enabled` so the flag survives storage round-trips. - The OTel profile form in the UI was reorganized into **Traces** and **Metrics** tabs. Trace-specific fields (collector URL, format, export timeout, request headers, content logging toggles) are nested under the Traces tab and hidden when `traces_enabled` is off. The Protocol selector was promoted to a shared connection setting above the tabs since both exporters use it. - Tab headers show a destructive badge when the tab contains a validation error, and the profile header shows a "Metrics only" badge when traces are disabled but metrics are enabled. - The E2E helper for enabling metrics export now clicks the Metrics tab before interacting with the toggle, since it is no longer the default active tab. - Added unit tests covering: default `TracesEnabled` behavior, metrics-only profile initialization, traces-enabled profile requiring `collector_url`, both-disabled no-op profile, and storage round-trip fidelity. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Plugin unit tests go test ./plugins/otel/... # UI cd ui pnpm i pnpm build ``` **Metrics-only profile config example:** ```json { "profiles": [ { "traces_enabled": false, "protocol": "http", "metrics_enabled": true, "metrics_endpoint": "otel-collector:4318" } ] } ``` Expected: profile initializes without error, no trace client is built, metrics exporter is active. **Existing traces-only config (no `traces_enabled` field):** should continue to work unchanged, defaulting `traces_enabled` to `true`. ## Breaking changes - [ ] Yes - [x] No Existing configs omitting `traces_enabled` default to `true` and behave identically to before. ## Security considerations No new secrets or auth surfaces introduced. The `collector_url` secret var handling is unchanged; it is simply no longer required when traces are disabled. ## 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
…lector (maximhq#5940) ## Summary Adds support for per-signal HTTP headers in OTel profiles, allowing separate headers to be sent exclusively to the trace endpoint or the metrics endpoint, in addition to the existing shared `headers` field. This is particularly useful when a metrics collector requires a signal-specific header (e.g. a Databricks table name) that should not be forwarded to the trace endpoint. ## Changes - Added `trace_headers` and `metrics_headers` fields to the `Profile` struct and `profileForStorage` struct, alongside the existing `headers` field. - `headers` continues to apply to both endpoints. `trace_headers` and `metrics_headers` are overlaid on top of the common headers at build time, with per-signal keys winning on collision. - Introduced `mergedResolvedHeaders` to merge common and per-signal header maps and resolve `env.VAR_NAME` references without mutating the inputs. - Extracted `redactHeaderMap` to eliminate duplicated redaction logic and applied it to all three header maps in `Redacted()`. - Updated the JSON schema (`config.schema.json`) with descriptions for all three header fields. - Updated the UI form to render separate `HeadersTable` inputs for common, trace-only, and metrics-only headers, each with descriptive labels and `FormDescription` text. - Updated the Zod schema and form serialization to include `trace_headers` and `metrics_headers`. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./plugins/otel/... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` Configure an OTel profile with all three header fields: ```json { "headers": { "Authorization": "env.OTEL_TOKEN" }, "trace_headers": { "X-Trace-Only": "trace-value" }, "metrics_headers": { "X-Databricks-Table": "my_table" } } ``` Verify that: - Trace requests include `Authorization` and `X-Trace-Only` but not `X-Databricks-Table`. - Metrics requests include `Authorization` and `X-Databricks-Table` but not `X-Trace-Only`. - `env.OTEL_TOKEN` is resolved from the environment on both endpoints. - Redacted config masks literal header values and preserves `env.` references across all three maps. ## Breaking changes - [ ] Yes - [x] No ## Security considerations All three header maps (`headers`, `trace_headers`, `metrics_headers`) are subject to the same redaction logic in `Redacted()`. Literal header values are masked and `env.` references are preserved as-is, consistent with prior behavior. No new secret storage paths are introduced. ## 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
…aximhq#5941) Adds `traces_enabled`, `trace_headers`, and `metrics_headers` fields to the OTEL plugin configuration (both single-profile and multi-profile shapes), enabling metrics-only OTEL profiles and per-signal header overrides. - Added `traces_enabled` boolean to OTEL config. When set to `false`, trace export is skipped and `collector_url` / `trace_type` are no longer required, allowing a metrics-only profile to be configured without a trace collector. - Added `trace_headers` and `metrics_headers` maps to OTEL config. The existing `headers` field continues to apply to both endpoints; `trace_headers` and `metrics_headers` are overlaid on top per-signal, with the more specific key winning on conflict. This supports cases where a collector requires a signal-specific header (e.g. a Databricks table name on the metrics endpoint only). - Updated validation logic in `_helpers.tpl` so that `collector_url` and `trace_type` are only required when `traces_enabled` is `true`, and `protocol` is only required when at least one of traces or metrics is enabled. - Updated `values.schema.json` conditional validation (`allOf`/`if`/`then`) to reflect the same rules: `collector_url`, `trace_type`, and `protocol` are gated on both `enabled` and `traces_enabled` not being `false`; `metrics_endpoint` and `protocol` are required together when `metrics_enabled` is `true`. - Updated `values.yaml` comments and `README.md` changelog to document the new fields. - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs Deploy the Helm chart with a metrics-only OTEL profile and verify that no trace collector URL is required: ```yaml bifrost: plugins: otel: enabled: true config: traces_enabled: false metrics_enabled: true metrics_endpoint: "http://otel-collector:4318/v1/metrics" protocol: "http" metrics_headers: x-databricks-table: "my_table" ``` ```sh helm template . -f values.yaml | grep -A 30 "otel" helm lint . ``` Verify that omitting `collector_url` with `traces_enabled: false` passes linting, and that omitting it with `traces_enabled: true` (default) still fails with the appropriate error message. N/A - [ ] Yes - [x] No N/A `trace_headers` and `metrics_headers` support the `env.VAR_NAME` prefix for injecting secrets from environment variables, consistent with the existing `headers` field. No new secret handling mechanisms are introduced. - [ ] 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
…cs list (maximhq#5942) ## Summary Documents two new OTel plugin capabilities: per-signal headers (`trace_headers` and `metrics_headers`) and a `traces_enabled` flag that enables a metrics-only mode where `collector_url` is not required. ## Changes - Added `traces_enabled` field documentation — when set to `false`, the trace client is never built and `collector_url`/`trace_type` become optional, enabling metrics-only profiles - Added `trace_headers` and `metrics_headers` fields — these are overlaid on top of the shared `headers` field for their respective endpoints, with per-signal values winning on key collision - Clarified that `headers` is sent to both trace and metrics endpoints, and that `protocol` is shared between both signals - Added a "Per-signal headers" section with a worked example showing `Authorization` shared via `headers` and `X-Databricks-Table` scoped to the metrics endpoint via `metrics_headers` - Added a "Metrics-only mode" section with a full JSON configuration example - Expanded the pushed metrics table to include `bifrost_cache_read_input_tokens_total`, `bifrost_cache_write_input_tokens_total`, `bifrost_cache_write_input_tokens_5m_total`, `bifrost_cache_write_input_tokens_1h_total`, `bifrost_request_retries`, and `mcp.client.operation.duration` - Added a note clarifying that an unreachable metrics endpoint never blocks Bifrost startup - Updated env-var substitution docs to include `trace_headers` and `metrics_headers` - Applied the same changes to both the `config-json` and Helm plugin reference pages ## 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 for the OTel plugin pages: - `docs/features/observability/otel.mdx` - `docs/deployment-guides/config-json/plugins.mdx` - `docs/deployment-guides/helm/plugins.mdx` Verify that: 1. The `traces_enabled: false` example produces a valid metrics-only config with no `collector_url` 2. The per-signal headers example correctly shows `Authorization` on both endpoints and `X-Databricks-Table` only on the metrics endpoint 3. All new metrics in the pushed metrics table are accurately described ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `trace_headers` and `metrics_headers` support the `env.` prefix for environment variable substitution, consistent with the existing `headers` field. No new secrets are stored in configuration. ## 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 Extends video logging and the log detail UI to fully support delete, list, download, and generation/remix/retrieve response types. Previously, delete responses were not routed to any log column, and the video detail view lacked support for delete output, base64-encoded video, and several generation metadata fields. ## Changes - In `applyNonStreamingOutputToEntry`, added routing for `VideoGenerationResponse`, `VideoDownloadResponse`, `VideoListResponse`, and `VideoDeleteResponse` into their respective log entry fields. `VideoGenerationResponse` is shared by generation, remix, and retrieve operations, so the request type is used as the discriminator to separate retrieve into its own column. - Added `video_delete_output` to the `videoOutput` expression in `logDetailView.tsx` so delete responses trigger the video detail panel. - Updated `VideoView` to handle `BifrostVideoDeleteOutput` as a distinct output type, rendering the video ID and deleted status. - Replaced the ad-hoc `requestType.toLowerCase().includes(...)` label logic with a lookup against `RequestTypeLabels`. - Added `getVideoSrc` to resolve a video source from either a URL or a base64 payload, and updated the video rendering loop to support multiple videos and base64-encoded content. - Added display of additional generation metadata fields: duration (`seconds`), size, and `remixed_from_video_id`. - Added `CopyableId` to video ID fields in the download and generation output sections. - Added the `ContentFilterInfo` type and `content_filter` field to `BifrostVideoGenerationOutput`. - Changed `seconds` from `number` to `string` on both `VideoObject` and `BifrostVideoGenerationOutput` to match the API shape. - Added tests covering all video response types (generation, remix, retrieve, download, list, delete) and verifying that content logging disabled suppresses video output. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./plugins/logging/... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` Trigger video generation, remix, retrieve, download, list, and delete requests and verify each response appears in the correct log column in the UI. Confirm that with content logging disabled, no video output fields are populated. ## Breaking changes - [ ] Yes - [x] No ## Security considerations No new auth, secrets, or PII surface area introduced. Video content is explicitly noted as not stored in logs for download responses. ## 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
## Summary Bedrock was rejecting documents with "The PDF specified was not valid" because the document format was always resolved to `"pdf"` regardless of the actual file type. Standard OpenAI clients encode the MIME type inside the data URL (e.g. `data:application/vnd.openxmlformats-officedocument.spreadsheetml.sheet;base64,...`) rather than in the `file_type` field, which was the only source previously consulted. This PR fixes format resolution for both the Chat and Responses paths, unifies the mapping logic, and corrects several related data URL parsing defects. Fixes maximhq#5472 ## Changes - Extracted a shared `bedrockDocumentFormat` helper in `utils.go` that maps MIME types and bare file extensions to Bedrock Converse document format strings, replacing two duplicated inline switch blocks that were missing most MIME types. - Format resolution now follows a priority chain: `file_type` → data URL media type → filename extension → `"pdf"` default. Previously only `file_type` was consulted. - `ParseDataURL` in `schemas/utils.go` is now a public function that correctly handles media type parameters (e.g. `;charset=utf-8`), uppercase media types, and payloads containing newlines. The old regex silently dropped any data URL whose header contained a parameter, causing the entire `"data:..."` string to be forwarded to Bedrock as the document payload. - Non-base64 data URLs (e.g. `data:text/plain,Hello%20World`) are now percent-decoded and their text content is populated in both `source.text` and `source.bytes` instead of being forwarded verbatim. - The Responses path (`responses.go`) previously ignored `file_url` entirely, emitting a document block with an empty source. It now fetches and inlines the bytes the same way the Chat path does, and propagates fetch errors rather than swallowing them. - `convertBifrostMessageToBedrockMessage` now returns an error instead of silently returning `nil` on conversion failure, so a missing turn is never silently dropped from the request. ## 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 go test ./core/providers/bedrock/... ./core/schemas/... ``` Key test cases added: - `TestDocumentFormatFromDataURL` — verifies that each supported MIME type embedded in a data URL resolves to the correct Bedrock format string and that the `data:...` prefix is stripped from `source.bytes`. - `TestDocumentFormatResolutionPrecedence` — verifies the `file_type` → data URL → filename extension → default priority chain. - `TestDocumentInlineTextDataURL` — verifies that non-base64 data URLs are percent-decoded and stored in both `source.text` and `source.bytes`. - `TestToBedrockResponsesRequest_DocumentFormatFromDataURL` — same format fix verified on the Responses path. - `TestToBedrockResponsesRequest_DocumentFileURLIsFetched` — verifies that an unreachable `file_url` surfaces as an error rather than producing an empty document block. - `TestParseDataURL` — unit tests for the new public `ParseDataURL` function covering parameters, uppercase, newlines in payload, and invalid inputs. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations `file_url` values are now fetched over the network on the Responses path (matching existing Chat path behaviour). The fetch is performed with the existing `providerUtils.FetchAndEncodeURL` helper, which is subject to the same controls already in place for image URL fetching. ## 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
Adds regression coverage for maximhq#5472, where Bedrock's document format converter defaulted every uploaded document to `format:"pdf"` regardless of the actual file type, causing AWS to reject non-PDF documents with `ValidationException`. This PR adds 14 end-to-end test cases to the provider harness collection covering the fixed behavior across both `/v1/chat/completions` and `/v1/responses`. - Added folder **42. Bedrock Document Uploads via OpenAI type:"file" (maximhq#5472)** to the provider harness collection with 14 test cases: - Cases 1–11 exercise `/v1/chat/completions` with XLSX, DOCX, CSV, PDF, TXT, and `file_url` inputs, covering format resolution by data URL media type, filename extension, explicit `file_type`, charset-parameterized data URLs, non-base64 percent-encoded data URLs, opaque media types, and streaming - Cases 12–14 pin the same invariants on `/v1/responses` `input_file` blocks (XLSX data URL, CSV data URL, `file_url`) - Every fixture embeds the token `BIFROST7788` so assertions confirm the document was actually parsed by Claude, not merely accepted - Updated `HARNESS_COVERAGE_BACKLOG.md` to mark the **Document input** item as partially covered (`[~]`), noting that the OpenAI `type:"file"` / Responses `input_file` path is now covered by folder 42, while a native Converse-shaped `document` block posted directly at `/bedrock/model/{id}/converse` remains uncovered - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs Import `tests/e2e/api/collections/provider-harness.json` into Postman and run folder **42. Bedrock Document Uploads via OpenAI type:"file" (maximhq#5472)** against a running Bifrost instance with Bedrock credentials configured. Each test asserts: - The response does not contain `"The PDF specified was not valid"`, `"could not be parsed as the specified format"`, or `"The document source bytes"` (the AWS rejection messages from the bug) - The response status is below 400 - For document-content cases, the model's reply includes `BIFROST7788`, confirming the document was read Before the fix, cases 1–3, 5–8, and 12–14 all returned a 400 `ValidationException`. - [x] No Closes maximhq#5472 None. Test fixtures contain only synthetic document content with no real credentials or PII. - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] 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
xAI's `grok-imagine` image generation API returns a `cost_in_usd_ticks` field in its usage object instead of token counts. Without this field on `ImageUsage`, the value was silently dropped during unmarshalling, causing the response to surface an empty `"usage":{}`.
Fixes maximhq#5498
## Changes
- Added `CostInUsdTicks *int64` to `ImageUsage` with `omitempty` so it is only serialized when present, leaving existing provider responses (OpenAI, Gemini, etc.) unaffected.
- Extended `DeepCopy` to allocate a new pointer for `CostInUsdTicks`, preserving the no-shared-pointers contract relied on by cost calculation logic.
- Added tests covering round-trip marshal/unmarshal of `cost_in_usd_ticks`, omission of the field when absent, and pointer independence after `DeepCopy`.
## 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
go test ./core/schemas/...
```
Expected: all three new tests pass — `TestImageUsage_CostInUsdTicksRoundTrip`, `TestImageUsage_CostInUsdTicksOmittedWhenAbsent`, and `TestImageUsage_DeepCopyCostInUsdTicks`.
## Breaking changes
- [ ] Yes
- [x] No
## Security considerations
No security implications. The new field is a cost/billing value returned by xAI and is passed through as-is.
## 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
…ximhq#5960) ## Summary Anthropic returns a 400 error with no error code when it rejects a `redacted_thinking` block containing a foreign or invalid payload. The existing `isEncryptedReasoningRejection` detection did not match this error format, causing the retry logic to miss these rejections and fail to strip the offending encrypted content before retrying. ## Changes - Extended `isEncryptedReasoningRejection` to also match Anthropic's `redacted_thinking`-specific rejection message: `"Invalid \`data\` in \`redacted_thinking\` block"`. - Added a comment explaining why this additional check is needed (Anthropic omits an error code and names the offending block in the message text instead). - Added three new test cases: - Confirms the `redacted_thinking` rejection is correctly detected. - Confirms that a `thinking` block signature rejection is intentionally **not** matched (since stripping encrypted content would not fix it and would cause an infinite retry loop). - Confirms that an unrelated Anthropic 400 mentioning `thinking` (e.g., invalid `budget_tokens`) is not incorrectly matched. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/... ``` The three new test cases in `TestIsEncryptedReasoningRejection` cover the added detection logic and the intentional non-matches. ## Breaking changes - [ ] Yes - [x] No ## Security considerations No auth, secrets, or PII implications. The change only affects error message pattern matching used to decide whether to strip encrypted reasoning content before retrying a request. ## 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
fcece06 to
0c4830b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@plugins/compat/main.go`:
- Around line 228-229: The shared typed context key is missing, causing the
compat override path to fail type checking. Add
BifrostContextKeyCompatCountTokensFallback to the typed context-key declarations
in core/schemas/bifrost.go, then ensure plugins/compat/main.go lines 228-229
reads that declared key and transports/bifrost-http/lib/ctx.go lines 582-611
clears and sets the same key.
🪄 Autofix
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: 95b94831-4cbd-47b4-bf98-3c6fb2d94a3b
📒 Files selected for processing (15)
core/schemas/bifrost.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.goplugins/compat/counttokensfallback.goplugins/compat/counttokensfallback_test.goplugins/compat/hooks_test.goplugins/compat/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/plugins.gotransports/config.schema.jsonui/app/workspace/config/views/compatibilityView.tsxui/lib/types/config.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- transports/bifrost-http/server/plugins.go
- core/schemas/bifrost.go
- framework/configstore/rdb.go
- transports/config.schema.json
- ui/lib/types/config.ts
- ui/app/workspace/config/views/compatibilityView.tsx
- framework/configstore/tables/clientconfig.go
- framework/configstore/clientconfig.go
- transports/bifrost-http/handlers/config.go
- framework/configstore/migrations.go
- plugins/compat/counttokensfallback_test.go
|
hi @akshaydeo, rebased onto latest dev and resolved the conflicts, CLA signed now too. Should be good to merge whenever you get a chance |
The merge-base changed after approval.
1eaa684 to
2ed4dd9
Compare
Summary
Add a graceful fallback for
count_tokenswhen the selected provider returnsunsupported_operation.This avoids failing outright for unsupported providers by returning a best-effort
response.input_tokensestimate derived from the normalized Responses request, while still preserving configured provider fallbacks and explicit operation allowlists.Changes
count_tokensrequestsAllowedRequests.count_tokens = falsebehaviorType of change
Affected areas
How to test
No new configs or environment variables were added.
Screenshots/Recordings
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Relates to #2902
Security considerations
No new auth, secret, or sandboxing behavior was introduced.
Checklist
docs/contributing/README.mdand followed the guidelines