Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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 (34)
✅ Files skipped from review due to trivial changes (25)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR updates provider capability handling and timeout defaults, adds logs dashboard and ranking support, improves transport error and path handling, expands workspace UI behavior, documents config reconciliation and integrations, and adds LiteLLM migration CLI code plus release automation. ChangesUnified platform update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Confidence Score: 4/5Safe to merge with awareness of the migration script's empty-intersection edge case and two minor incomplete capModel migrations in OpenAI reasoning effort estimation. The scripts/litellm-to-bifrost/virtualkey.go — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming BifrostRequest\nrequest.Model = alias key] --> B{ResolvedAlias\nin ctx?}
B -- Yes --> C{ra.Config.ModelName\nnon-empty?}
C -- Yes --> D[capModel = ModelName\ne.g. claude-opus-4-8]
C -- No --> E{ra.Config.ModelID\nnon-empty?}
E -- Yes --> F[capModel = ModelID\ne.g. claude-opus-4-8-20251101]
E -- No --> G[capModel = fallbackModel]
B -- No --> G
D --> H[Capability gating\nIsAdaptiveOnlyThinkingModel\nIsGrokReasoningModel\nsupportsThinkingConfig\netc.]
F --> H
G --> H
H --> I[Wire model unchanged\nrequest.Model sent to upstream API]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Incoming BifrostRequest\nrequest.Model = alias key] --> B{ResolvedAlias\nin ctx?}
B -- Yes --> C{ra.Config.ModelName\nnon-empty?}
C -- Yes --> D[capModel = ModelName\ne.g. claude-opus-4-8]
C -- No --> E{ra.Config.ModelID\nnon-empty?}
E -- Yes --> F[capModel = ModelID\ne.g. claude-opus-4-8-20251101]
E -- No --> G[capModel = fallbackModel]
B -- No --> G
D --> H[Capability gating\nIsAdaptiveOnlyThinkingModel\nIsGrokReasoningModel\nsupportsThinkingConfig\netc.]
F --> H
G --> H
H --> I[Wire model unchanged\nrequest.Model sent to upstream API]
Reviews (6): Last reviewed commit: "fix: bedrock signauture fix for minimax ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+79
to
+80
| names := intersectModelNames(key.Models, teamModels, orgModels) | ||
| plan.ProviderConfigs, plan.UnmappedModels = resolveProviderKeyConfigs(names, keyModelIdx, wildcardKeys, allProviders) |
Contributor
There was a problem hiding this comment.
intersectModelNames receives the raw strings from key.Models (e.g. "openai/*", "anthropic/*") and compares them against llmAllModelSentinels, which only contains "*", "all-proxy-models", and "all-team-models". A provider-scoped wildcard like "openai/*" is not a sentinel, so it passes into resolveProviderKeyConfigs as a specific model name. There it is looked up in keyModelIdx, which is keyed by trimmed model names (e.g. "gpt-4o") — keyModelIdx["openai/*"] returns nil, and the entry lands in UnmappedModels.
The result is that a VK restricted to "openai/*" is migrated with no provider configs. Depending on how Bifrost treats an empty ProviderConfigs slice, the VK could end up with either broader access than the original (allow-all) or broken access. Either outcome misrepresents the LiteLLM intent. The fix would be to strip the provider prefix from each entry in the incoming lists before checking sentinels, or to add provider-scoped wildcards like "<provider>/*" to the sentinel set.
Contributor
There was a problem hiding this comment.
The fix looks correct. The new block in resolveProviderKeyConfigs handles provider-scoped wildcards properly:
if n != "*" && trimModelPrefix(n) == "*" {
provider, _, _ := strings.Cut(n, "/")
if !providerSet[provider] {
unmapped = append(unmapped, n)
continue
}
ensureProvider(provider)
fullWildcard[provider] = true
continue
}"openai/*" is now detected via trimModelPrefix(n) == "*", grants key_ids=["*"] for that provider (via fullWildcard), and unknown providers fall through to unmapped correctly. This accurately preserves the LiteLLM intent.
![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: 13
🧹 Nitpick comments (1)
scripts/litellm-to-bifrost/main.go (1)
17-18: ⚡ Quick winMake the migration timeout configurable (or disable hard timeout by default).
A fixed 5-minute deadline for the entire multi-phase migration can abort healthy large runs. This is better as an explicit env-configurable timeout.
💡 Suggested direction
-ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) +ctx := context.Background() +cancel := func() {} +if raw := strings.TrimSpace(os.Getenv("MIGRATION_TIMEOUT")); raw != "" { + d, err := time.ParseDuration(raw) + if err != nil { + log.Fatalf("invalid MIGRATION_TIMEOUT %q: %v", raw, err) + } + var c context.CancelFunc + ctx, c = context.WithTimeout(ctx, d) + cancel = c +} defer cancel()🤖 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 `@scripts/litellm-to-bifrost/main.go` around lines 17 - 18, The context timeout is hardcoded to 5 minutes in the context.WithTimeout call, which can abort healthy large migration runs. Replace the hardcoded 5*time.Minute duration with a configurable value that reads from an environment variable (e.g., MIGRATION_TIMEOUT). Parse this environment variable at the start of the main function and provide a sensible default (either a much longer duration or no timeout) if the variable is not set. Use this parsed duration value instead of the hardcoded 5*time.Minute constant in the context.WithTimeout call.
🤖 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/bifrost.go`:
- Around line 523-529: The error message matching logic in the bifrost.go file
uses case-sensitive string comparisons with strings.Contains to check for
expected error messages. To prevent equivalent error messages with different
capitalization from being incorrectly treated as unexpected errors, normalize
the errMsg variable to lowercase before performing the strings.Contains checks
against "no keys found", "not supported", and other error message patterns. This
ensures consistent matching regardless of how the error message is capitalized
in the actual bifrostErr.Error.Message response.
In `@scripts/litellm-to-bifrost/bifrost.go`:
- Around line 517-527: The sendJSON method currently returns nil (success) for
all HTTP 409 Conflict responses without validation, which silently accepts
incompatible pre-existing entities and masks migration failures. Instead of
blindly treating all 409s as success in the status check after the doRequest
call in sendJSON, implement logic to validate whether the conflict is due to a
compatible pre-existing entity or represents an incompatible state that should
cause an error. Consider examining the response body (out) or adding additional
validation to distinguish between acceptable and unacceptable conflict scenarios
before deciding whether to return nil or an error.
In `@scripts/litellm-to-bifrost/litellm/user.go`:
- Around line 65-66: The pagination break condition at line 65 in the user fetch
loop uses `page >= pageResp.TotalPages` which will cause premature exit if
TotalPages is zero or missing, silently truncating users. Replace this condition
to prioritize checking if the current page response is empty
(len(pageResp.Users) == 0) as the primary break signal, and only use the
TotalPages check when it has a valid non-zero value. This ensures the loop
continues fetching pages until an empty response is received, rather than
relying on a potentially missing TotalPages value.
In `@scripts/litellm-to-bifrost/litellm/virtualkey.go`:
- Around line 44-45: The pagination loop stopping condition at line 44 uses
`page >= pageResp.TotalPages` as a primary stopping criterion, which will exit
prematurely if TotalPages is zero or missing from the API response, causing only
the first page of virtual keys to be processed. Refactor the stopping condition
to prioritize checking whether keys were actually returned in the current page
response (the `len(pageResp.Keys) == 0` check) and only use the TotalPages
comparison as a secondary safeguard, or add explicit handling for the edge case
where TotalPages is zero or undefined to ensure all available pages are fetched
regardless of the TotalPages value.
In `@scripts/litellm-to-bifrost/main.go`:
- Around line 245-247: The LiteLLMUsersToBifrostUsers function and
printUserReport function are logging full email addresses in the migration
output, exposing PII unnecessarily. Refactor these functions to redact, hash, or
mask the email addresses before including them in the plans report or any log
output. Apply this same redaction pattern consistently across all locations
where user emails are logged or printed in the migration output, including the
LiteLLMUsersToBifrostUsers and printUserReport functions and any other locations
where user data is displayed during the migration process.
- Around line 59-64: The requireEnv function only validates that an environment
variable is set but does not check if its value is empty, allowing critical
configuration like URLs or API keys to be empty strings and fail later with
unclear errors. Modify the requireEnv function to add an additional validation
check after the os.LookupEnv call: if the returned val is an empty string, call
log.Fatalf with a message indicating that the environment variable must not be
empty, ensuring only valid non-empty configuration values pass validation at
startup.
In `@scripts/litellm-to-bifrost/model.go`:
- Around line 637-645: The alias resolution in the loop processing
litellmCfg.RouterSettings.ModelGroupAlias only performs a single pass and does
not transitively resolve chained aliases (for example, A -> B -> C where B
itself is an alias). To fix this, implement transitive resolution by either
wrapping the existing loop in an outer iteration that repeats until a fixed
point is reached (no new refs are added), or by creating a helper function that
recursively resolves each alias target to its final destination before calling
addRef. Ensure that when processing the ModelGroupAlias entries, if the target
is itself another alias, you follow the chain to the ultimate target model
reference.
In `@scripts/litellm-to-bifrost/organization.go`:
- Around line 26-27: The regex pattern in litellmDurationRe permits zero values
(e.g., `0m`) but should only accept positive integers. Modify the regex pattern
to use [1-9]\d* instead of \d+ for the first capturing group, which will match
positive integers starting from 1 and exclude zero. Apply this same change to
any similar duration parsing logic in the ranges mentioned (lines 120-131) that
use similar patterns.
In `@scripts/litellm-to-bifrost/scripts/delete_bifrost_entities.sh`:
- Line 18: The usage example comment references the script filename with a
double underscore as delete_bifrost__entities.sh, but the actual script filename
uses a single underscore. Correct the typo in this comment by changing
delete_bifrost__entities.sh to delete_bifrost_entities.sh to match the actual
script name and prevent user confusion when copying the example command.
- Around line 269-275: The `|| true` wrappers in the main section suppress all
failure exit codes from the deletion functions (delete_all_virtual_keys,
delete_all_model_configs, delete_all_teams, delete_all_customers,
delete_all_users, delete_all_provider_keys, delete_all_providers), causing the
script to exit successfully even when deletions fail. Remove the `|| true`
wrappers from each of the seven conditional statements so that if any deletion
function fails, the failure exit code is preserved and the script exits with a
non-zero status to indicate the overall failure.
In `@scripts/litellm-to-bifrost/scripts/delete_entities.sh`:
- Line 6: Update the header comment that references the migration command path.
The comment currently mentions `./scripts/migration/litellm` but should be
updated to reference the correct path `scripts/litellm-to-bifrost` to reflect
where the tool actually resides in this PR.
- Around line 80-85: The deletion operations in functions delete_all_keys,
delete_all_teams, and delete_all_organizations currently mask failures by using
the || echo pattern, which logs the error but allows the script to continue
successfully. Remove the || operator and the echo statement from the curl
commands (at lines 80-85, 109-114, 138-143, and 258-263) so that curl failures
cause the function to exit immediately with a non-zero status. Additionally,
remove the || true pattern from the main function to allow error codes from the
deletion functions to propagate up, enabling automation to detect partial
cleanup failures.
In `@scripts/litellm-to-bifrost/scripts/seed_models.sh`:
- Line 222: The parameter expansion `${DRY_RUN:+(dry-run) }` in the echo
statement treats any non-empty value of DRY_RUN as true, so even when DRY_RUN is
set to "0" (representing a false state), it will still print (dry-run). Replace
this parameter expansion with a conditional that explicitly checks if DRY_RUN is
set to a truthy value such as "true" or "1", rather than just checking if the
variable is non-empty. This ensures (dry-run) is only printed when dry-run mode
is actually active.
---
Nitpick comments:
In `@scripts/litellm-to-bifrost/main.go`:
- Around line 17-18: The context timeout is hardcoded to 5 minutes in the
context.WithTimeout call, which can abort healthy large migration runs. Replace
the hardcoded 5*time.Minute duration with a configurable value that reads from
an environment variable (e.g., MIGRATION_TIMEOUT). Parse this environment
variable at the start of the main function and provide a sensible default
(either a much longer duration or no timeout) if the variable is not set. Use
this parsed duration value instead of the hardcoded 5*time.Minute constant in
the context.WithTimeout call.
🪄 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: fd368b3f-d955-481c-9d2b-45e16aca7965
⛔ Files ignored due to path filters (1)
scripts/litellm-to-bifrost/go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
.github/workflows/helm-release.ymlcore/bifrost.gocore/bifrost_test.gocore/providers/bedrock/transport_test.gocore/providers/fireworks/fireworks_test.gocore/providers/mistral/ocr_test.gocore/providers/mistral/transcription_test.gocore/providers/utils/utils.gocore/schemas/provider.gocore/schemas/serialization_test.goplugins/governance/resolver.goplugins/governance/resolver_test.goscripts/litellm-to-bifrost/bifrost.goscripts/litellm-to-bifrost/go.modscripts/litellm-to-bifrost/litellm/crypto.goscripts/litellm-to-bifrost/litellm/db.goscripts/litellm-to-bifrost/litellm/litellm.goscripts/litellm-to-bifrost/litellm/models.goscripts/litellm-to-bifrost/litellm/organization.goscripts/litellm-to-bifrost/litellm/team.goscripts/litellm-to-bifrost/litellm/user.goscripts/litellm-to-bifrost/litellm/virtualkey.goscripts/litellm-to-bifrost/main.goscripts/litellm-to-bifrost/model.goscripts/litellm-to-bifrost/modelconformance_test.goscripts/litellm-to-bifrost/organization.goscripts/litellm-to-bifrost/scripts/create_organizations.shscripts/litellm-to-bifrost/scripts/create_teams.shscripts/litellm-to-bifrost/scripts/create_virtual_keys.shscripts/litellm-to-bifrost/scripts/delete_bifrost_entities.shscripts/litellm-to-bifrost/scripts/delete_entities.shscripts/litellm-to-bifrost/scripts/seed_models.shscripts/litellm-to-bifrost/team.goscripts/litellm-to-bifrost/user.goscripts/litellm-to-bifrost/utils.goscripts/litellm-to-bifrost/virtualkey.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.jsonui/app/workspace/providers/dialogs/addNewCustomProviderSheet.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/lib/constants/config.ts
Comment on lines
+523
to
+529
| errMsg := "" | ||
| if bifrostErr.Error != nil { | ||
| errMsg = bifrostErr.Error.Message | ||
| } | ||
| isExpected := strings.Contains(errMsg, "no keys found") || | ||
| strings.Contains(errMsg, "not supported") || | ||
| errType == "provider_blocked" |
Contributor
There was a problem hiding this comment.
Normalize error-message casing before expected-error matching.
strings.Contains here is case-sensitive, so equivalent messages with different capitalization can be treated as unexpected and incorrectly promote a per-provider failure.
Suggested fix
- errMsg := ""
+ errMsg := ""
if bifrostErr.Error != nil {
errMsg = bifrostErr.Error.Message
}
- isExpected := strings.Contains(errMsg, "no keys found") ||
- strings.Contains(errMsg, "not supported") ||
+ errMsgLower := strings.ToLower(errMsg)
+ isExpected := strings.Contains(errMsgLower, "no keys found") ||
+ strings.Contains(errMsgLower, "not supported") ||
errType == "provider_blocked"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| errMsg := "" | |
| if bifrostErr.Error != nil { | |
| errMsg = bifrostErr.Error.Message | |
| } | |
| isExpected := strings.Contains(errMsg, "no keys found") || | |
| strings.Contains(errMsg, "not supported") || | |
| errType == "provider_blocked" | |
| errMsg := "" | |
| if bifrostErr.Error != nil { | |
| errMsg = bifrostErr.Error.Message | |
| } | |
| errMsgLower := strings.ToLower(errMsg) | |
| isExpected := strings.Contains(errMsgLower, "no keys found") || | |
| strings.Contains(errMsgLower, "not supported") || | |
| errType == "provider_blocked" |
🤖 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/bifrost.go` around lines 523 - 529, The error message matching logic in
the bifrost.go file uses case-sensitive string comparisons with strings.Contains
to check for expected error messages. To prevent equivalent error messages with
different capitalization from being incorrectly treated as unexpected errors,
normalize the errMsg variable to lowercase before performing the
strings.Contains checks against "no keys found", "not supported", and other
error message patterns. This ensures consistent matching regardless of how the
error message is capitalized in the actual bifrostErr.Error.Message response.
Contributor
|
Too many files changed for review. ( |
## Summary Fixes a bug where OTEL plugin headers were being overwritten with redacted placeholder values when saving a plugin configuration. After the multi-profile change, header values stored as plain strings inside the `profiles` array were not being restored from the database before saving, causing real credentials to be replaced with masked values like `****`. ## Changes - Extracted `restoreRedactedValue` as a standalone recursive helper, replacing the inline logic in `restoreRedactedFromExisting`. This allows the restoration logic to descend into both nested maps and slices. - Added slice traversal support (index-aligned) so that elements within arrays like the OTEL `profiles` array are individually checked and restored. - Added plain-string redaction detection so that header values stored as raw strings (rather than `EnvVar` objects) are also restored from the existing DB config when they carry a redaction artifact. Empty strings are intentionally left as-is to allow clearing a value. - Added `TestRestoreRedacted_OTELProfilesHeaders` to cover both failure modes: slice traversal and plain-string secret restoration. Also asserts that genuinely new (non-redacted) values pass through unchanged. ## 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/... ``` Verify that saving an OTEL plugin configuration with multiple profiles, after a GET that returns redacted header values, does not overwrite the stored credentials in the database. Confirm that providing a genuinely new header value still persists correctly. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations This fix ensures that redacted credential placeholders returned to the client are never written back over real secrets stored in the database. The restoration logic only replaces values that are confirmed redaction artifacts; empty strings and non-redacted values are always passed through as-is, preserving the ability to clear a credential intentionally. ## 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 When `SkipKeySelection` is set (e.g. Claude Code OAuth/max mode), governance was being bypassed entirely — even for requests that included a virtual key. This meant users supplying a virtual key alongside an OAuth flow were not subject to governance controls. ## Changes - `PreLLMHook` now only skips governance when `SkipKeySelection` is true **and** no virtual key is present. If a virtual key is supplied, full governance runs regardless of the skip flag. - Moved the `virtualKeyValue` extraction to before the `SkipKeySelection` check so it can be used in the condition, removing the need for a second extraction later in the function. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test 1. Send a request with `SkipKeySelection` set and **no** virtual key — governance should be bypassed as before. 2. Send a request with `SkipKeySelection` set and a **valid virtual key** — governance should run and enforce policies. 3. Send a request with `SkipKeySelection` set and an **invalid/unauthorized virtual key** — governance should reject the request. ```sh go test ./plugins/governance/... ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations This fix closes a governance bypass where OAuth users who also supplied a virtual key could circumvent access controls. Requests with a virtual key now always go through full governance validation, regardless of the `SkipKeySelection` flag. ## 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 Fixes a duplicate `message_start` event emitted on the Anthropic passthrough responses stream path. When a single upstream `message_start` is received, it expands into two bifrost responses: `response.created` and `response.in_progress`. The raw upstream frame was previously attached to the last chunk (`in_progress`), which caused the transport to emit two `message_start` frames — a lossy synthesized one from `created` and the raw-forwarded one from `in_progress`. The fix attaches the raw frame to `response.created` instead, ensuring exactly one `message_start` is forwarded with all upstream fields intact (including `service_tier` and `inference_geo`). ## Changes - When attaching the upstream raw response to a bifrost response chunk, the default "last chunk" rule is now overridden for `message_start` expansions: the raw is attached to `response.created` rather than `response.in_progress`. - Added `TestResponsesStream_MessageStart_NoDuplicateOnPassthrough` to explicitly guard this fix, verifying that the correct placement emits exactly one verbatim `message_start` and that the old placement produces the duplicate behavior. ## 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/anthropic/... -run TestResponsesStream_MessageStart_NoDuplicateOnPassthrough -v ``` Expected output: both subtests pass — `raw on created emits one full message_start` confirms a single verbatim frame, and `raw on in_progress (old behavior) duplicates message_start` confirms the old rule produced two frames. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations No security implications. ## 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
core/providers/bedrock/responses.go (1)
2225-2241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
reasoning.summaryin the max-token Anthropic Bedrock path.This branch builds
thinkingwithoutdisplay, soreasoning.summary: "none"or"auto"is ignored whenevermax_tokensis present. The effort-only branch below already mapsSummaryto Bedrockdisplay; keep the same request contract here too.Proposed fix
if schemas.IsAnthropicModelFamily(ctx, bifrostReq.Model) { if anthropic.IsAdaptiveOnlyThinkingModel(capModel) { - bedrockReq.AdditionalModelRequestFields.Set("thinking", map[string]any{ + thinkingConfig := map[string]any{ "type": "adaptive", - }) + } + if bifrostReq.Params.Reasoning.Summary != nil { + if *bifrostReq.Params.Reasoning.Summary == "none" { + thinkingConfig["display"] = "omitted" + } else { + thinkingConfig["display"] = "summarized" + } + } else { + thinkingConfig["display"] = "summarized" + } + bedrockReq.AdditionalModelRequestFields.Set("thinking", thinkingConfig) // Preserve a co-present effort — these models support effort, // and the budget is otherwise dropped. if bifrostReq.Params.Reasoning.Effort != nil && *bifrostReq.Params.Reasoning.Effort != "none" { setOutputConfigField(bedrockReq.AdditionalModelRequestFields, "effort", anthropic.MapBifrostEffortToAnthropic(*bifrostReq.Params.Reasoning.Effort)) } } else { @@ - bedrockReq.AdditionalModelRequestFields.Set("thinking", map[string]any{ + thinkingConfig := map[string]any{ "type": "enabled", "budget_tokens": tokenBudget, - }) + } + if bifrostReq.Params.Reasoning.Summary != nil { + if *bifrostReq.Params.Reasoning.Summary == "none" { + thinkingConfig["display"] = "omitted" + } else { + thinkingConfig["display"] = "summarized" + } + } + bedrockReq.AdditionalModelRequestFields.Set("thinking", thinkingConfig) } } else if schemas.IsNovaModelFamily(ctx, bifrostReq.Model) {🤖 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` around lines 2225 - 2241, The else block handling the max_tokens path for reasoning is not preserving the reasoning.summary parameter when setting up the thinking configuration, while the if block for adaptive-only models already maps Summary to Bedrock display. After setting the thinking object with type and budget_tokens in the else branch, add a check for bifrostReq.Params.Reasoning.Summary similar to the existing check for Effort, and use the appropriate anthropic mapping function to convert the summary value to Bedrock format, then set it in the thinking configuration using setOutputConfigField with the display key to maintain consistency with the effort-only path.ui/app/workspace/skills-repo/components/shared.tsx (1)
884-933:⚠️ Potential issue | 🔴 CriticalPass numeric panel sizes to
ResizablePanel, not string percentages.The
defaultSize,minSize, andmaxSizeprops forResizablePanelexpect numeric values (percentage 0-100), not CSS percentage strings. The codebase already follows this pattern correctly inpromptsView.tsx(e.g.,defaultSize={20}) but the new code inshared.tsx(lines 885, 933) andskillEditForm.tsx(lines 260, 322) uses string percentages like"28%", which mismatches both the TypeScript type definition and the established codebase convention.Proposed fix
- <ResizablePanel defaultSize="28%" minSize="18%" maxSize="50%" className="flex min-h-0 flex-col gap-2"> + <ResizablePanel defaultSize={28} minSize={18} maxSize={50} className="flex min-h-0 flex-col gap-2">Apply the same fix to
skillEditForm.tsxlines 260 and 322.🤖 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 `@ui/app/workspace/skills-repo/components/shared.tsx` around lines 884 - 933, The defaultSize, minSize, and maxSize props of the ResizablePanel component expect numeric values representing percentages (0-100), not string percentages. In the shared.tsx file, change the ResizablePanel props from string values like "28%", "18%", "50%", "72%", and "30%" to their numeric equivalents (28, 18, 50, 72, 30). Apply the same fix to skillEditForm.tsx at lines 260 and 322 where ResizablePanel is also using string percentage values instead of numeric values, matching the established pattern used in promptsView.tsx.core/providers/gemini/gemini_test.go (1)
356-390:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert conversion errors before using
result.At Line 356 and Line 398,
erris not checked beforeresultis dereferenced. If conversion fails, this can panic and hide the real failure reason.Suggested fix
result, err := gemini.ToGeminiChatCompletionRequest(nil, &schemas.BifrostChatRequest{ // ... }) +require.NoError(t, err) // ... result, err := gemini.ToGeminiChatCompletionRequest(nil, &schemas.BifrostChatRequest{ // ... }) +require.NoError(t, err)Also applies to: 398-421
🤖 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/gemini/gemini_test.go` around lines 356 - 390, Add error checking immediately after calling ToGeminiChatCompletionRequest before using the result object. Use require.NoError(t, err) right after the function call to validate that the conversion succeeded and avoid potential panics when dereferencing result. Apply this same fix to both locations mentioned where result is used without error validation.core/providers/openai/chat.go (2)
141-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
capModelwhen deriving reasoning effort frommax_tokens.The explicit-effort path uses the canonical model, but the
max_tokenspath still reads defaults fromreq.Model. For aliases/custom model names, this can map the same budget to the wrong effort tier.🐛 Proposed fix
- maxCompletionTokens := utils.GetMaxOutputTokensOrDefault(req.Model, DefaultCompletionMaxTokens) + maxCompletionTokens := utils.GetMaxOutputTokensOrDefault(capModel, DefaultCompletionMaxTokens)🤖 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/openai/chat.go` around lines 141 - 159, The normalizeReasoningEffort function uses the canonical model capModel when normalizing native effort values, but in the max_tokens path it still uses req.Model when calling utils.GetMaxOutputTokensOrDefault. For consistency and to handle model aliases correctly, change the call to utils.GetMaxOutputTokensOrDefault to use capModel instead of req.Model in the else-if block where req.ChatParameters.Reasoning.MaxTokens is not nil.
78-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
capModelfor Vertex Mistral compatibility.This branch still gates Mistral compatibility on the wire/alias model. If an alias resolves to a Vertex-hosted Mistral model,
applyMistralCompatibility()is skipped and incompatible OpenAI-style params can be sent upstream.🐛 Proposed fix
- if schemas.IsMistralModel(bifrostReq.Model) { + if schemas.IsMistralModel(capModel) { openaiReq.applyMistralCompatibility()🤖 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/openai/chat.go` around lines 78 - 83, The Mistral compatibility check in the Vertex case branch is using bifrostReq.Model (the wire/alias model) instead of capModel (the resolved capability model) in the IsMistralModel function call. This causes the applyMistralCompatibility method to be skipped when an alias resolves to a Vertex-hosted Mistral model. Update the condition from checking schemas.IsMistralModel(bifrostReq.Model) to schemas.IsMistralModel(capModel) to ensure compatibility transformations are applied based on the actual resolved model rather than the alias name.core/providers/openai/responses.go (1)
226-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the canonical model for budget-derived reasoning effort.
The explicit
Effortpath now normalizes withcapModel, but theMaxTokenspath still gets output-token defaults fromreq.Model(the wire/original model). Alias-routed requests can therefore estimate the wrong effort even though the rest of the capability checks use the resolved model.🐛 Proposed fix
- maxOutputTokens := utils.GetMaxOutputTokensOrDefault(req.Model, DefaultCompletionMaxTokens) + maxOutputTokens := utils.GetMaxOutputTokensOrDefault(capModel, DefaultCompletionMaxTokens)🤖 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/openai/responses.go` around lines 226 - 229, The MaxTokens path in the reasoning effort estimation is using req.Model to get output token defaults, but it should use the canonical/resolved model (capModel) instead, just like the explicit Effort path does for normalization. Replace req.Model with capModel in the utils.GetMaxOutputTokensOrDefault call within the else-if block that handles ResponsesParameters.Reasoning.MaxTokens to ensure alias-routed requests estimate effort consistently using the resolved model rather than the original wire model.ui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsx (1)
18-23:⚠️ Potential issue | 🟡 MinorGate the 409 conflict return on handler existence and remove the
anycast.
onConflictis optional, but line 75 returns early for all 409 errors. When no handler is supplied, the error is silently swallowed instead of falling through toonError. Additionally, line 75 uses an explicitanycast which violates the typing guidelines.Condition the early return on
onConflictbeing defined and replace theanycast with a runtime type check:🐛 Proposed fix
if (cancelledRef.current) return; const errMsg = getErrorMessage(err); - if ((err as any)?.status === 409) { + const errorStatus = + typeof err === "object" && err !== null && "status" in err + ? (err as { status: unknown }).status + : undefined; + if (errorStatus === 409 && onConflict) { setStatus("input"); setErrorMessage(null); - onConflict?.(errMsg); + onConflict(errMsg); return; }🤖 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 `@ui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsx` around lines 18 - 23, The onConflict callback in MCPHeadersAuthorizerProps is optional, but the error handler at line 75 unconditionally returns early for all 409 status code responses without checking if onConflict is actually defined, causing errors to be silently swallowed when no handler is supplied. Additionally, that same section uses an explicit any cast that violates typing guidelines. Modify the 409 error handling logic to first check if onConflict is defined before executing the early return, and replace the any cast with a proper runtime type check to ensure 409 errors fall through to the onError handler when onConflict is not provided.Source: Coding guidelines
🧹 Nitpick comments (8)
ui/app/workspace/skills-repo/dialogs/skillVersionDialog.tsx (1)
25-76: ⚡ Quick winRemove duplicated query/pagination state from
SkillVersionsPopover.After extracting
SkillVersionsList, the wrapper still runs its ownuseListSkillVersionsQuery+ accumulator + observer logic, which is no longer rendered and can double request work when this popover is used. Keep onlyopenstate in the popover and delegate data loading entirely toSkillVersionsList.Suggested cleanup
export function SkillVersionsPopover({ skillId, servingVersion, onSelectVersion, }: { skillId: string; servingVersion: string; onSelectVersion: (version: SkillVersionSummary) => void; }) { const [open, setOpen] = useState(false); - const [search, setSearch] = useState(""); - const debouncedSearch = useDebouncedValue(search, 300); - const [offset, setOffset] = useState(0); - const [accumulated, setAccumulated] = useState<SkillVersionSummary[]>([]); - - const { data, isFetching, isError } = useListSkillVersionsQuery( - { - id: skillId, - limit: PAGE_SIZE, - offset, - search: debouncedSearch || undefined, - }, - { skip: !open }, - ); - - useEffect(() => { - setOffset(0); - setAccumulated([]); - }, [debouncedSearch, skillId]); - - useEffect(() => { - if (!data) return; - setAccumulated((prev) => { - if (offset === 0) return data.versions; - const seen = new Set(prev.map((v) => v.id)); - return [...prev, ...data.versions.filter((v) => !seen.has(v.id))]; - }); - }, [data, offset]); - - const total = data?.total ?? 0; - const hasMore = accumulated.length < total; - - const sentinelRef = useRef<HTMLDivElement | null>(null); - useEffect(() => { - const node = sentinelRef.current; - if (!node || !open || !hasMore || isFetching) return; - const observer = new IntersectionObserver( - (entries) => { - if (entries[0]?.isIntersecting) { - setOffset((o) => o + PAGE_SIZE); - } - }, - { threshold: 1 }, - ); - observer.observe(node); - return () => observer.disconnect(); - }, [open, hasMore, isFetching]); return (🤖 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 `@ui/app/workspace/skills-repo/dialogs/skillVersionDialog.tsx` around lines 25 - 76, Remove all query and pagination state management from the skillVersionDialog component since this logic is now handled by the extracted SkillVersionsList component. Delete the search, setSearch, debouncedSearch, offset, setOffset, and accumulated state variables along with their associated useState hooks. Remove the useListSkillVersionsQuery hook call, all three useEffect hooks that manage search reset, data accumulation, and the IntersectionObserver infinite scroll, and the sentinelRef variable. Keep only the open state and ensure the component passes the necessary props (skillId and open) to the SkillVersionsList component to delegate all data loading and pagination responsibilities to it.ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx (1)
157-163: Use proper RTK Query error typing instead ofany; this pattern already exists in baseApi.ts.Line 157 uses
(error as any)?.status === 409. The proposed verbose narrowing check doesn't improve safety. Instead, import and use RTK Query's error types (FetchBaseQueryError | SerializedError) for proper typing, or reference how baseApi.ts handles this same issue at line 80 where it also castsresult.error as anybefore checking status codes. Either align with the established codebase pattern or introduce proper RTK Query type imports.🤖 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 `@ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx` around lines 157 - 163, The error handling code at line 157 uses `(error as any)?.status === 409` which loses type safety. Replace this improper `any` cast by importing RTK Query's proper error types (FetchBaseQueryError and SerializedError) and use those types to narrow the error object before checking the status property. Alternatively, align with the established pattern already present in baseApi.ts at line 80 where similar error status checking is performed. The goal is to maintain type safety when checking the 409 conflict status in the error handling block that invokes onConflict callback.Source: Coding guidelines
core/providers/openai/responses_test.go (1)
187-187: ⚡ Quick winAdd coverage for the new non-nil context path.
All updated calls pass
nil, so the newResolveCanonicalModel(ctx, ...)behavior is only tested in fallback mode. Please add a case with a resolved alias inBifrostContextthat verifies capability gating uses the canonical model whilereq.Modelstays unchanged.As per coding guidelines,
core/**changes should include “table-driven tests for behavior changes.”Also applies to: 246-246, 281-281, 425-425, 546-546, 1681-1681, 1753-1753, 1879-1879
🤖 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/openai/responses_test.go` at line 187, The test coverage for ToOpenAIResponsesRequest is incomplete because all existing test calls pass nil context, leaving the new ResolveCanonicalModel(ctx, ...) behavior untested when context is non-nil. Add table-driven test cases that provide a non-nil BifrostContext with a resolved model alias to verify that capability gating correctly uses the canonical model while the original req.Model value remains unchanged. Apply similar test coverage improvements to all affected call sites of ToOpenAIResponsesRequest throughout the test file.Source: Coding guidelines
transports/bifrost-http/handlers/logging_test.go (1)
139-139: ⚡ Quick winExercise the registered dashboard route.
Line 139 calls
getDashboarddirectly, so this test can pass even if the new/api/logs/dashboardRegisterRoutesentry is missing or misregistered. Add one router-backed success path that callsRegisterRoutesand dispatches the request through the router.As per coding guidelines, "HTTP handlers are dependency-injected structs with
RegisterRoutes, and route middleware should be composed throughlib.ChainMiddlewares."🤖 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 `@transports/bifrost-http/handlers/logging_test.go` at line 139, The test currently calls the getDashboard handler directly, which bypasses the router and doesn't verify that the route is properly registered. Add a new test path that creates a router, calls RegisterRoutes to register the dashboard route, then makes an actual HTTP request to the `/api/logs/dashboard` endpoint through the router using a test HTTP client. This ensures the route registration and middleware composition through the router are properly exercised, not just the handler function itself.Source: Coding guidelines
ui/lib/hooks/useTimezonePreference.ts (3)
4-4: ⚡ Quick winUse the
@/lib/*alias import for consistency.Switch
../timezonesto@/lib/timezonesto match established UI import conventions across the codebase.Suggested diff
-import { getSupportedTimezones } from "../timezones"; +import { getSupportedTimezones } from "`@/lib/timezones`";Based on learnings, UI files should prefer alias imports using
@/...and avoid relative imports forui/**/*.ts(x).🤖 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 `@ui/lib/hooks/useTimezonePreference.ts` at line 4, The import statement for getSupportedTimezones in useTimezonePreference.ts is using a relative path import instead of the alias convention. Replace the relative import path ../timezones with the alias path `@/lib/timezones` in the getSupportedTimezones import statement to maintain consistency with established UI import conventions across the codebase.Source: Learnings
1-1: ⚡ Quick winRemove the
use clientdirective in this UI hook module.This repo’s UI stack guidance is to avoid introducing Next.js-specific directives in
ui/**; this line adds framework-specific noise without functional benefit here.Based on learnings, “Within the Bifrost UI code under ui/ (TanStack React Router), do not introduce Next.js-specific constructs such as adding ‘use client’ directives.”
🤖 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 `@ui/lib/hooks/useTimezonePreference.ts` at line 1, Remove the "use client" directive from the top of the useTimezonePreference hook file. The ui directory follows TanStack React Router conventions and should not include Next.js-specific constructs like "use client" directives, as they add framework-specific noise without functional benefit to this hook module.Source: Learnings
44-48: ⚡ Quick winValidate timezone before persisting in
setTimezone.The setter currently accepts any string and persists it immediately; validating input here prevents invalid values from future callsites from polluting UI state and storage.
Suggested diff
const setTimezone = useCallback((tz: string) => { + if (!isValidTimezone(tz)) return; setTimezoneState(tz); try { localStorage.setItem(STORAGE_KEY, tz); } catch {🤖 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 `@ui/lib/hooks/useTimezonePreference.ts` around lines 44 - 48, The `setTimezone` callback function should validate the timezone string parameter before persisting it to state and storage. Add validation logic at the beginning of `setTimezone` to check if the `tz` parameter is a valid timezone value before calling `setTimezoneState(tz)` and `localStorage.setItem(STORAGE_KEY, tz)`. Only proceed with state and storage updates if the timezone passes validation, preventing invalid values from polluting both UI state and localStorage.ui/app/workspace/mcp-registry/views/mcpClientForm.tsx (1)
299-302: Replace theanycast with a typed guard for the 409 status check.Line 299 uses an unnecessary
anycast; theerrorparameter from the catch block has typeunknownand can be narrowed using a type guard instead, eliminating theanycast entirely and improving type safety.♻️ Proposed fix
- if ((error as any)?.status === 409) { + const errorStatus = + typeof error === "object" && error !== null && "status" in error + ? (error as { status: unknown }).status + : undefined; + if (errorStatus === 409) { setError("name", { message: getErrorMessage(error) }); return; }🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx` around lines 299 - 302, The error handling block in mcpClientForm.tsx uses an unnecessary any cast when checking the 409 status code on the error object. Replace the (error as any)?.status check with a proper type guard that narrows the unknown error type without resorting to any casting. Create a type guard function or use an inline check to determine if the error object has a status property of type number, then verify if it equals 409. This approach eliminates the unsafe any cast while maintaining proper type safety and clarity.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.
Inline comments:
In `@docs/openapi/paths/management/logging.yaml`:
- Around line 842-847: The metadata_key parameter definition uses a literal
parameter name with a placeholder syntax (metadata_<key>) which will be treated
as a literal query parameter name by OpenAPI generators rather than representing
dynamic parameter keys. Remove this parameter from the reusable parameter
definitions in the logging.yaml file and instead document the dynamic metadata_*
convention directly in the endpoint descriptions and examples where it is
actually used, or alternatively refactor the metadata filtering to use a
properly supported map-shaped query parameter format that OpenAPI can accurately
represent.
In `@ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx`:
- Around line 348-350: The conflict detection logic in
mcpLibraryInstallSheet.tsx uses an unsafe type cast `(error as any)?.status ===
409` which bypasses type safety. Since the error from RTK Query's unwrap() is of
unknown type, replace the any cast with a proper type guard function or inline
type narrowing that checks if the error object has a status property before
accessing it. Create a type predicate function or use conditional type checking
logic to safely determine if the error is an object with a numeric status
property equal to 409, maintaining type safety throughout the error handling
path.
In `@ui/app/workspace/skills-repo/components/shared.tsx`:
- Around line 116-134: Add data-testid attributes to the interactive buttons in
the description toggle section to enable reliable E2E testing. For the "Show
less" button that appears when expanded is true and isOverflowing is true, add
an appropriate data-testid attribute. Similarly, add a data-testid attribute to
the "Show more" button that appears when expanded is false and isOverflowing is
true. Ensure each test ID clearly identifies the button's purpose for test
automation.
- Around line 370-376: The byTail fallback logic in the relative link resolution
uses files.find() which returns the first arbitrary match when multiple files
share the same trailing path segment, potentially opening the wrong file.
Replace the find() call with filter() to get all matching files and only return
the file path if exactly one unique match exists, ensuring the fallback only
succeeds when the trailing path segment is unambiguous, consistent with how the
bare-filename fallback operates.
In `@ui/app/workspace/skills-repo/dialogs/skillVersionDialog.tsx`:
- Around line 180-182: The CommandEmpty component is rendering even when isError
is true, causing both the empty-state message and error-state message to display
simultaneously. Add !isError to the condition that checks !isFetching &&
accumulated.length === 0 for the CommandEmpty rendering to prevent the
empty-state from appearing when an error has occurred. Apply this same fix to
the other similar condition also mentioned at lines 210-212.
In `@ui/app/workspace/skills-repo/forms/skillEditForm.tsx`:
- Around line 284-289: Update the data-testid attributes to follow the 3-part
convention pattern (entity-element-qualifier). The Input element with
placeholder "Search files..." currently has data-testid="sidebar-search" which
is only 2 parts; update it to follow the pattern (e.g., "sidebar-search-input").
Additionally, locate the new version popover Cancel button around lines 813-815
and add a missing data-testid attribute following the same 3-part convention
pattern (e.g., "popover-version-cancel" or similar entity-element-qualifier
format).
- Around line 294-304: The FileManagerSection component is using
selectedFileIndex to track which file is selected, but when onRemoveFile is
called (which invokes form.removeFile), the selectedFileIndex is not being
updated. This causes the selection to become misaligned: deleting the selected
file silently switches to another file at the same index, and deleting an
earlier file loses the current selection due to index shifting. Add logic to the
onRemoveFile handler that adjusts selectedFileIndex appropriately - either by
clearing it when the selected file is removed, or by decrementing it when a file
earlier in the array is removed.
- Around line 90-107: The issue is that validateSkillForm can return version and
skill_md_body errors, but these fields are either hidden (version is in a
popover) or not being validated inline (skill_md_body), causing the early return
to block the popover from opening. Modify the error handling in the condition
where errors.length > 0: add form.validateField call for skill_md_body field,
filter the errors array to exclude version field errors before using errors.find
to check for blocking errors, and only return early if there are blocking errors
on visible pane fields (name, description, extra_frontmatter, metadata). This
allows the popover to open even when there are version validation errors while
still showing inline errors for all fields.
- Around line 446-449: The custom overlay dims the page but does not trap
keyboard focus, allowing users to Tab into controls behind the popover. Add the
`modal={true}` prop to all three Popover Root components (located at lines 466,
490, and 518) to enable Radix UI's built-in focus management. This will
automatically trap focus within the popover when it is open, ensuring keyboard
users cannot navigate to elements behind the overlay.
In `@ui/components/ui/datePickerWithRange.tsx`:
- Around line 188-190: The timezone change handler in the 320-344 range is
calling onDateTimeUpdate for all date selections, including relative periods
which are explicitly marked as timezone-independent at lines 188-190. This
causes relative periods to be silently converted to absolute ranges. Add a check
in the timezone change handler to detect whether the current selection is a
predefined/relative period by checking if predefinedPeriod is set, and skip the
onDateTimeUpdate call for relative periods since they don't require timezone
adjustments. Only emit the update for absolute date range selections.
In `@ui/components/ui/resizable.tsx`:
- Line 39: The className string for the resizable handle contains
focus-visible:ring-0 which removes the focus ring and
focus-visible:outline-hidden which hides the outline, making the interactive
separator invisible to keyboard users when focused. Replace focus-visible:ring-0
with focus-visible:ring-2 (or similar appropriate ring class) to restore the
visible focus indicator, and remove or replace focus-visible:outline-hidden with
an appropriate focus styling class that maintains visual feedback for keyboard
navigation.
---
Outside diff comments:
In `@core/providers/bedrock/responses.go`:
- Around line 2225-2241: The else block handling the max_tokens path for
reasoning is not preserving the reasoning.summary parameter when setting up the
thinking configuration, while the if block for adaptive-only models already maps
Summary to Bedrock display. After setting the thinking object with type and
budget_tokens in the else branch, add a check for
bifrostReq.Params.Reasoning.Summary similar to the existing check for Effort,
and use the appropriate anthropic mapping function to convert the summary value
to Bedrock format, then set it in the thinking configuration using
setOutputConfigField with the display key to maintain consistency with the
effort-only path.
In `@core/providers/gemini/gemini_test.go`:
- Around line 356-390: Add error checking immediately after calling
ToGeminiChatCompletionRequest before using the result object. Use
require.NoError(t, err) right after the function call to validate that the
conversion succeeded and avoid potential panics when dereferencing result. Apply
this same fix to both locations mentioned where result is used without error
validation.
In `@core/providers/openai/chat.go`:
- Around line 141-159: The normalizeReasoningEffort function uses the canonical
model capModel when normalizing native effort values, but in the max_tokens path
it still uses req.Model when calling utils.GetMaxOutputTokensOrDefault. For
consistency and to handle model aliases correctly, change the call to
utils.GetMaxOutputTokensOrDefault to use capModel instead of req.Model in the
else-if block where req.ChatParameters.Reasoning.MaxTokens is not nil.
- Around line 78-83: The Mistral compatibility check in the Vertex case branch
is using bifrostReq.Model (the wire/alias model) instead of capModel (the
resolved capability model) in the IsMistralModel function call. This causes the
applyMistralCompatibility method to be skipped when an alias resolves to a
Vertex-hosted Mistral model. Update the condition from checking
schemas.IsMistralModel(bifrostReq.Model) to schemas.IsMistralModel(capModel) to
ensure compatibility transformations are applied based on the actual resolved
model rather than the alias name.
In `@core/providers/openai/responses.go`:
- Around line 226-229: The MaxTokens path in the reasoning effort estimation is
using req.Model to get output token defaults, but it should use the
canonical/resolved model (capModel) instead, just like the explicit Effort path
does for normalization. Replace req.Model with capModel in the
utils.GetMaxOutputTokensOrDefault call within the else-if block that handles
ResponsesParameters.Reasoning.MaxTokens to ensure alias-routed requests estimate
effort consistently using the resolved model rather than the original wire
model.
In `@ui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsx`:
- Around line 18-23: The onConflict callback in MCPHeadersAuthorizerProps is
optional, but the error handler at line 75 unconditionally returns early for all
409 status code responses without checking if onConflict is actually defined,
causing errors to be silently swallowed when no handler is supplied.
Additionally, that same section uses an explicit any cast that violates typing
guidelines. Modify the 409 error handling logic to first check if onConflict is
defined before executing the early return, and replace the any cast with a
proper runtime type check to ensure 409 errors fall through to the onError
handler when onConflict is not provided.
In `@ui/app/workspace/skills-repo/components/shared.tsx`:
- Around line 884-933: The defaultSize, minSize, and maxSize props of the
ResizablePanel component expect numeric values representing percentages (0-100),
not string percentages. In the shared.tsx file, change the ResizablePanel props
from string values like "28%", "18%", "50%", "72%", and "30%" to their numeric
equivalents (28, 18, 50, 72, 30). Apply the same fix to skillEditForm.tsx at
lines 260 and 322 where ResizablePanel is also using string percentage values
instead of numeric values, matching the established pattern used in
promptsView.tsx.
---
Nitpick comments:
In `@core/providers/openai/responses_test.go`:
- Line 187: The test coverage for ToOpenAIResponsesRequest is incomplete because
all existing test calls pass nil context, leaving the new
ResolveCanonicalModel(ctx, ...) behavior untested when context is non-nil. Add
table-driven test cases that provide a non-nil BifrostContext with a resolved
model alias to verify that capability gating correctly uses the canonical model
while the original req.Model value remains unchanged. Apply similar test
coverage improvements to all affected call sites of ToOpenAIResponsesRequest
throughout the test file.
In `@transports/bifrost-http/handlers/logging_test.go`:
- Line 139: The test currently calls the getDashboard handler directly, which
bypasses the router and doesn't verify that the route is properly registered.
Add a new test path that creates a router, calls RegisterRoutes to register the
dashboard route, then makes an actual HTTP request to the `/api/logs/dashboard`
endpoint through the router using a test HTTP client. This ensures the route
registration and middleware composition through the router are properly
exercised, not just the handler function itself.
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Around line 299-302: The error handling block in mcpClientForm.tsx uses an
unnecessary any cast when checking the 409 status code on the error object.
Replace the (error as any)?.status check with a proper type guard that narrows
the unknown error type without resorting to any casting. Create a type guard
function or use an inline check to determine if the error object has a status
property of type number, then verify if it equals 409. This approach eliminates
the unsafe any cast while maintaining proper type safety and clarity.
In `@ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx`:
- Around line 157-163: The error handling code at line 157 uses `(error as
any)?.status === 409` which loses type safety. Replace this improper `any` cast
by importing RTK Query's proper error types (FetchBaseQueryError and
SerializedError) and use those types to narrow the error object before checking
the status property. Alternatively, align with the established pattern already
present in baseApi.ts at line 80 where similar error status checking is
performed. The goal is to maintain type safety when checking the 409 conflict
status in the error handling block that invokes onConflict callback.
In `@ui/app/workspace/skills-repo/dialogs/skillVersionDialog.tsx`:
- Around line 25-76: Remove all query and pagination state management from the
skillVersionDialog component since this logic is now handled by the extracted
SkillVersionsList component. Delete the search, setSearch, debouncedSearch,
offset, setOffset, and accumulated state variables along with their associated
useState hooks. Remove the useListSkillVersionsQuery hook call, all three
useEffect hooks that manage search reset, data accumulation, and the
IntersectionObserver infinite scroll, and the sentinelRef variable. Keep only
the open state and ensure the component passes the necessary props (skillId and
open) to the SkillVersionsList component to delegate all data loading and
pagination responsibilities to it.
In `@ui/lib/hooks/useTimezonePreference.ts`:
- Line 4: The import statement for getSupportedTimezones in
useTimezonePreference.ts is using a relative path import instead of the alias
convention. Replace the relative import path ../timezones with the alias path
`@/lib/timezones` in the getSupportedTimezones import statement to maintain
consistency with established UI import conventions across the codebase.
- Line 1: Remove the "use client" directive from the top of the
useTimezonePreference hook file. The ui directory follows TanStack React Router
conventions and should not include Next.js-specific constructs like "use client"
directives, as they add framework-specific noise without functional benefit to
this hook module.
- Around line 44-48: The `setTimezone` callback function should validate the
timezone string parameter before persisting it to state and storage. Add
validation logic at the beginning of `setTimezone` to check if the `tz`
parameter is a valid timezone value before calling `setTimezoneState(tz)` and
`localStorage.setItem(STORAGE_KEY, tz)`. Only proceed with state and storage
updates if the timezone passes validation, preventing invalid values from
polluting both UI state and localStorage.
🪄 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: 2e269ad9-4345-4a76-a3bc-e6cb3c55b7c6
⛔ Files ignored due to path filters (5)
docs/media/cli/claude-desktop-developer-gateway.pngis excluded by!**/*.pngdocs/media/cli/claude-desktop-gateway-config-idp.pngis excluded by!**/*.pngdocs/media/cli/claude-desktop-gateway-config-vk.pngis excluded by!**/*.pngscripts/litellm-to-bifrost/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (157)
.github/workflows/helm-release.ymlcore/bifrost.gocore/bifrost_test.gocore/internal/llmtests/passthrough_api.gocore/providers/anthropic/advisor_test.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/requestbuilder.gocore/providers/anthropic/responses.gocore/providers/anthropic/utils.gocore/providers/azure/azure.gocore/providers/bedrock/batch.gocore/providers/bedrock/batch_test.gocore/providers/bedrock/bedrock.gocore/providers/bedrock/bedrock_test.gocore/providers/bedrock/cache_points_test.gocore/providers/bedrock/chat.gocore/providers/bedrock/mantle.gocore/providers/bedrock/responses.gocore/providers/bedrock/s3.gocore/providers/bedrock/s3_test.gocore/providers/bedrock/transport_test.gocore/providers/bedrock/utils.gocore/providers/fireworks/fireworks_test.gocore/providers/gemini/chat.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/mistral/ocr_test.gocore/providers/mistral/transcription_test.gocore/providers/openai/advisor_filter_test.gocore/providers/openai/chat.gocore/providers/openai/chat_test.gocore/providers/openai/openai.gocore/providers/openai/responses.gocore/providers/openai/responses_marshal_test.gocore/providers/openai/responses_test.gocore/providers/utils/utils.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.gocore/schemas/account_test.gocore/schemas/provider.gocore/schemas/serialization_test.gocore/schemas/utils.godocs/cli-agents/claude-desktop.mdxdocs/deployment-guides/config-json.mdxdocs/deployment-guides/config-json/governance.mdxdocs/deployment-guides/config-json/plugins.mdxdocs/deployment-guides/config-json/providers.mdxdocs/deployment-guides/config-json/schema-reference.mdxdocs/deployment-guides/config-json/source-of-truth.mdxdocs/deployment-guides/config-json/storage.mdxdocs/deployment-guides/helm/governance.mdxdocs/docs.jsondocs/enterprise/datadog-connector.mdxdocs/openapi/openapi.yamldocs/openapi/paths/management/logging.yamldocs/openapi/schemas/management/logging.yamldocs/quickstart/gateway/setting-up.mdxframework/configstore/rdb.goframework/logstore/migrations.goframework/logstore/tables.gohelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlplugins/governance/main.goplugins/governance/modelprovidergovernance_test.goplugins/governance/resolver.goplugins/governance/resolver_test.goscripts/litellm-to-bifrost/bifrost.goscripts/litellm-to-bifrost/go.modscripts/litellm-to-bifrost/litellm/crypto.goscripts/litellm-to-bifrost/litellm/db.goscripts/litellm-to-bifrost/litellm/litellm.goscripts/litellm-to-bifrost/litellm/models.goscripts/litellm-to-bifrost/litellm/organization.goscripts/litellm-to-bifrost/litellm/team.goscripts/litellm-to-bifrost/litellm/user.goscripts/litellm-to-bifrost/litellm/virtualkey.goscripts/litellm-to-bifrost/main.goscripts/litellm-to-bifrost/model.goscripts/litellm-to-bifrost/modelconformance_test.goscripts/litellm-to-bifrost/organization.goscripts/litellm-to-bifrost/scripts/create_organizations.shscripts/litellm-to-bifrost/scripts/create_teams.shscripts/litellm-to-bifrost/scripts/create_virtual_keys.shscripts/litellm-to-bifrost/scripts/delete_bifrost_entities.shscripts/litellm-to-bifrost/scripts/delete_entities.shscripts/litellm-to-bifrost/scripts/seed_models.shscripts/litellm-to-bifrost/team.goscripts/litellm-to-bifrost/user.goscripts/litellm-to-bifrost/utils.goscripts/litellm-to-bifrost/virtualkey.gotests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtests/e2e/api/collections/bifrost-api-management.postman_collection.jsontransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/logging_test.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/handlers/provider_keys.gotransports/bifrost-http/handlers/skills_serving.gotransports/bifrost-http/handlers/skills_serving_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.jsonui/app/workspace/custom-pricing/overrides/page.tsxui/app/workspace/custom-pricing/overrides/scopedPricingOverridesView.tsxui/app/workspace/dashboard/page.tsxui/app/workspace/dashboard/utils/exportUtils.tsui/app/workspace/governance/views/customerSheet.tsxui/app/workspace/governance/views/teamSheet.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/views/columns.tsxui/app/workspace/logs/views/logsHeaderView.tsxui/app/workspace/mcp-logs/page.tsxui/app/workspace/mcp-logs/views/columns.tsxui/app/workspace/mcp-logs/views/mcpHeaderView.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpHeadersAuthorizer.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/app/workspace/model-limits/page.tsxui/app/workspace/model-limits/views/modelLimitsTable.tsxui/app/workspace/model-limits/views/modelLimitsView.tsxui/app/workspace/providers/dialogs/addNewCustomProviderSheet.tsxui/app/workspace/providers/fragments/networkFormFragment.tsxui/app/workspace/providers/views/providerKeyForm.tsxui/app/workspace/routing-rules/page.tsxui/app/workspace/routing-rules/views/routingRulesTable.tsxui/app/workspace/routing-rules/views/routingRulesView.tsxui/app/workspace/skills-repo/components/fileManagerView.tsxui/app/workspace/skills-repo/components/filePreview.tsxui/app/workspace/skills-repo/components/shared.tsxui/app/workspace/skills-repo/components/skillDetailsView.tsxui/app/workspace/skills-repo/components/skillListView.tsxui/app/workspace/skills-repo/dialogs/skillVersionDialog.tsxui/app/workspace/skills-repo/dialogs/versionDetailsDialog.tsxui/app/workspace/skills-repo/forms/skillEditForm.tsxui/app/workspace/skills-repo/page.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/table/hooks/useColumnConfig.tsui/components/ui/command.tsxui/components/ui/datePickerWithRange.tsxui/components/ui/resizable.tsxui/components/ui/scrollArea.tsxui/components/ui/treeView.tsxui/lib/constants/config.tsui/lib/hooks/useTimezonePreference.tsui/lib/timezones.tsui/lib/types/logs.tsui/package.json
💤 Files with no reviewable changes (1)
- ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
✅ Files skipped from review due to trivial changes (25)
- core/providers/openai/advisor_filter_test.go
- ui/app/workspace/custom-pricing/overrides/page.tsx
- core/providers/mistral/ocr_test.go
- ui/app/workspace/routing-rules/page.tsx
- ui/lib/timezones.ts
- docs/deployment-guides/helm/governance.mdx
- ui/package.json
- docs/deployment-guides/config-json/plugins.mdx
- core/bifrost_test.go
- ui/app/workspace/skills-repo/dialogs/versionDetailsDialog.tsx
- tests/cmd/seed/go.mod
- framework/configstore/rdb.go
- helm-charts/bifrost/values.yaml
- docs/deployment-guides/config-json/source-of-truth.mdx
- core/providers/anthropic/utils.go
- docs/deployment-guides/config-json/providers.mdx
- ui/app/workspace/skills-repo/components/filePreview.tsx
- ui/lib/constants/config.ts
- core/providers/fireworks/fireworks_test.go
- docs/deployment-guides/config-json/schema-reference.mdx
- transports/bifrost-http/handlers/plugins_test.go
- docs/deployment-guides/config-json.mdx
- docs/cli-agents/claude-desktop.mdx
- docs/deployment-guides/config-json/storage.mdx
- ui/app/workspace/skills-repo/page.tsx
🚧 Files skipped from review as they are similar to previous changes (42)
- scripts/litellm-to-bifrost/go.mod
- scripts/litellm-to-bifrost/litellm/team.go
- ui/app/workspace/providers/dialogs/addNewCustomProviderSheet.tsx
- tests/cmd/seedvks/go.mod
- scripts/litellm-to-bifrost/litellm/organization.go
- scripts/litellm-to-bifrost/litellm/virtualkey.go
- scripts/litellm-to-bifrost/utils.go
- tests/cmd/e2eseed/go.mod
- plugins/governance/resolver.go
- core/providers/mistral/transcription_test.go
- scripts/litellm-to-bifrost/litellm/crypto.go
- .github/workflows/helm-release.yml
- scripts/litellm-to-bifrost/scripts/create_virtual_keys.sh
- core/schemas/provider.go
- plugins/governance/resolver_test.go
- core/providers/utils/utils.go
- docs/deployment-guides/config-json/governance.mdx
- scripts/litellm-to-bifrost/litellm/models.go
- core/bifrost.go
- transports/bifrost-http/handlers/plugins.go
- tests/e2e/api/collections/bifrost-api-management.postman_collection.json
- scripts/litellm-to-bifrost/team.go
- transports/bifrost-http/lib/config_test.go
- scripts/litellm-to-bifrost/litellm/user.go
- scripts/litellm-to-bifrost/scripts/create_organizations.sh
- core/providers/bedrock/transport_test.go
- scripts/litellm-to-bifrost/scripts/delete_bifrost_entities.sh
- scripts/litellm-to-bifrost/scripts/delete_entities.sh
- scripts/litellm-to-bifrost/organization.go
- scripts/litellm-to-bifrost/virtualkey.go
- docs/quickstart/gateway/setting-up.mdx
- scripts/litellm-to-bifrost/user.go
- scripts/litellm-to-bifrost/litellm/db.go
- framework/logstore/migrations.go
- scripts/litellm-to-bifrost/litellm/litellm.go
- scripts/litellm-to-bifrost/scripts/seed_models.sh
- core/schemas/serialization_test.go
- scripts/litellm-to-bifrost/modelconformance_test.go
- scripts/litellm-to-bifrost/scripts/create_teams.sh
- scripts/litellm-to-bifrost/model.go
- scripts/litellm-to-bifrost/main.go
- scripts/litellm-to-bifrost/bifrost.go
Comment on lines
+842
to
+847
| metadata_key: | ||
| name: metadata_<key> | ||
| in: query | ||
| description: Metadata filter where <key> is the metadata key to match | ||
| schema: | ||
| type: string |
Contributor
There was a problem hiding this comment.
Avoid documenting a dynamic query key as a literal OpenAPI parameter.
name: metadata_<key> becomes a literal parameter in generated clients, while the description says callers should replace <key>. Either expose a real map-shaped query parameter the handler supports, or move this dynamic metadata_* convention into endpoint descriptions/examples instead of the reusable parameter list.
🤖 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 `@docs/openapi/paths/management/logging.yaml` around lines 842 - 847, The
metadata_key parameter definition uses a literal parameter name with a
placeholder syntax (metadata_<key>) which will be treated as a literal query
parameter name by OpenAPI generators rather than representing dynamic parameter
keys. Remove this parameter from the reusable parameter definitions in the
logging.yaml file and instead document the dynamic metadata_* convention
directly in the endpoint descriptions and examples where it is actually used, or
alternatively refactor the metadata filtering to use a properly supported
map-shaped query parameter format that OpenAPI can accurately represent.
Comment on lines
+348
to
+350
| if ((error as any)?.status === 409) { | ||
| setError("name", { message: getErrorMessage(error) }); | ||
| return; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the target file
fd -t f "mcpLibraryInstallSheet.tsx"Repository: maximhq/bifrost
Length of output: 130
🏁 Script executed:
# Get the file size to determine how to read it
wc -l ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxRepository: maximhq/bifrost
Length of output: 134
🏁 Script executed:
# Read the specific section around lines 348-350 plus context
sed -n '340,360p' ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxRepository: maximhq/bifrost
Length of output: 522
🏁 Script executed:
# Check for PR stack information and recent changes
git log --oneline -20 2>/dev/null || echo "No git history available"Repository: maximhq/bifrost
Length of output: 1270
🏁 Script executed:
# Get broader context around the error handling - check the full function
sed -n '300,370p' ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxRepository: maximhq/bifrost
Length of output: 2675
🏁 Script executed:
# Search for other uses of (error as any) in the file and similar patterns
rg "error as any|\.status ===" ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx -nRepository: maximhq/bifrost
Length of output: 180
🏁 Script executed:
# Check for getErrorMessage function to understand error type expectations
rg -B 5 -A 10 "getErrorMessage" ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx | head -40Repository: maximhq/bifrost
Length of output: 1795
🏁 Script executed:
# Look for other similar error.status checks in the codebase
rg "error.*\.status" ui/ -t ts -t tsx --max-count 10 -A 2Repository: maximhq/bifrost
Length of output: 87
🏁 Script executed:
# Let's search for error.status patterns in TypeScript/React files
rg "error.*\.status" ui/ --type ts -A 2 | head -50Repository: maximhq/bifrost
Length of output: 3325
🏁 Script executed:
# Check the function signature and error type in catch block
sed -n '285,360p' ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxRepository: maximhq/bifrost
Length of output: 2480
🏁 Script executed:
# Look for error type annotations or interfaces related to API errors
rg "export.*error|type.*Error|interface.*Error" ui/lib/types/ | head -20Repository: maximhq/bifrost
Length of output: 554
🏁 Script executed:
# Check how getErrorMessage is defined and what types it expects
rg -B 2 -A 8 "export.*getErrorMessage|function getErrorMessage" ui/lib/Repository: maximhq/bifrost
Length of output: 701
🏁 Script executed:
# Let's see how virtualKeySheet handles the error check - it doesn't use (as any)
sed -n '1,50p' ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx && echo "---" && rg -B 5 -A 5 "error\?\.status === 409" ui/app/workspace/virtual-keys/views/virtualKeySheet.tsxRepository: maximhq/bifrost
Length of output: 1801
🏁 Script executed:
# Check the full getErrorMessage implementation
sed -n '1,100p' ui/lib/store/apis/baseApi.ts | grep -A 30 "getErrorMessage"Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Look at the createMCPClient function and its error type
rg -B 5 -A 15 "createMCPClient" ui/lib/store/ | head -60Repository: maximhq/bifrost
Length of output: 1493
🏁 Script executed:
# Check RTK Query error types or error handling patterns
rg "FetchBaseQueryError|SerializedError" ui/ --max-count 5 -B 2 -A 2Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check mcp-sessions/auth/page.tsx to see the proper pattern it uses
sed -n '1,100p' ui/app/workspace/mcp-sessions/auth/page.tsx | grep -B 5 -A 5 "status =.*error"Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Count how many files have (error as any)?.status pattern
rg "\(error as any\)\?\.status" ui/ --count-matchesRepository: maximhq/bifrost
Length of output: 247
🏁 Script executed:
# Check if there's a common error type in RTK Query or the store
rg "type.*Error|interface.*Error" ui/lib/store/apis/baseApi.ts -A 5 | head -40Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Look at what RTK Query mutation hooks return for errors
rg "catch.*error" ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx -B 2 -A 10Repository: maximhq/bifrost
Length of output: 359
🏁 Script executed:
# Check if there's a pattern where errors are typed or how createMCPClient is used elsewhere
rg -B 10 "createMCPClient.*unwrap" ui/ | head -50Repository: maximhq/bifrost
Length of output: 3000
🏁 Script executed:
# Let's see how the error is handled in customerSheet and teamSheet without casting
rg -B 5 -A 5 "error\?\.status === 409" ui/app/workspace/governance/views/Repository: maximhq/bifrost
Length of output: 1628
🏁 Script executed:
# Check if there's a specific RTK Query error type definition in the codebase
rg "SerializedError|FetchBaseQueryError|ApiError" ui/lib --max-count 10 -B 2 -A 2Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Let's check the exact pattern in mcp-sessions/auth/page.tsx that uses proper typing
rg -B 10 -A 5 "const status = \(error as" ui/app/workspace/mcp-sessions/auth/page.tsxRepository: maximhq/bifrost
Length of output: 731
🏁 Script executed:
# Count how many catch blocks use "error: any" in similar files
rg "catch \(error: any\)" ui/app/workspace/ | wc -lRepository: maximhq/bifrost
Length of output: 61
🏁 Script executed:
# Final check - verify the exact context of the catch block to confirm error type
sed -n '340,365p' ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxRepository: maximhq/bifrost
Length of output: 943
Replace any cast in conflict detection with a typed status guard.
Line 348 uses (error as any)?.status === 409, which drops type safety in the error-handling path. The catch block receives an unknown error type from RTK Query's unwrap(), and proper type narrowing should be used before accessing the status field.
Proposed fix
- if ((error as any)?.status === 409) {
+ const status =
+ typeof error === "object" &&
+ error !== null &&
+ "status" in error
+ ? (error as { status?: unknown }).status
+ : undefined;
+ if (status === 409) {
setError("name", { message: getErrorMessage(error) });
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ((error as any)?.status === 409) { | |
| setError("name", { message: getErrorMessage(error) }); | |
| return; | |
| const status = | |
| typeof error === "object" && | |
| error !== null && | |
| "status" in error | |
| ? (error as { status?: unknown }).status | |
| : undefined; | |
| if (status === 409) { | |
| setError("name", { message: getErrorMessage(error) }); | |
| return; |
🤖 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 `@ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx`
around lines 348 - 350, The conflict detection logic in
mcpLibraryInstallSheet.tsx uses an unsafe type cast `(error as any)?.status ===
409` which bypasses type safety. Since the error from RTK Query's unwrap() is of
unknown type, replace the any cast with a proper type guard function or inline
type narrowing that checks if the error object has a status property before
accessing it. Create a type predicate function or use conditional type checking
logic to safely determine if the error is an object with a numeric status
property equal to 409, maintaining type safety throughout the error handling
path.
Source: Coding guidelines
Comment on lines
+116
to
+134
| {expanded && isOverflowing && ( | ||
| <button | ||
| type="button" | ||
| onClick={() => setExpanded(false)} | ||
| className="ml-1 cursor-pointer text-xs font-medium text-blue-600 transition-colors hover:underline dark:text-blue-400" | ||
| > | ||
| Show less | ||
| </button> | ||
| )} | ||
| </p> | ||
| {!expanded && isOverflowing && ( | ||
| <button | ||
| type="button" | ||
| onClick={() => setExpanded(true)} | ||
| className="bg-card absolute right-0 bottom-0 cursor-pointer pl-8 pr-4 text-xs font-medium text-blue-600 transition-colors hover:underline dark:text-blue-400" | ||
| > | ||
| <span className="from-card pointer-events-none absolute top-0 right-full h-full w-8 bg-gradient-to-l to-transparent" /> | ||
| Show more | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add stable selectors for the new interactive controls.
Several new buttons/triggers lack data-testid values, including the description toggles, raw view/copy controls, helper info trigger, markdown file-link/open-link dialog actions, and body tab triggers. These flows will be hard to cover reliably in Playwright. As per coding guidelines, “Add data-testid to all new interactive elements in React components for E2E test compatibility.”
Also applies to: 207-214, 251-263, 297-303, 438-483, 511-516
🤖 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 `@ui/app/workspace/skills-repo/components/shared.tsx` around lines 116 - 134,
Add data-testid attributes to the interactive buttons in the description toggle
section to enable reliable E2E testing. For the "Show less" button that appears
when expanded is true and isOverflowing is true, add an appropriate data-testid
attribute. Similarly, add a data-testid attribute to the "Show more" button that
appears when expanded is false and isOverflowing is true. Ensure each test ID
clearly identifies the button's purpose for test automation.
Source: Coding guidelines
Comment on lines
+370
to
+376
| // Fall back to matching by trailing path segments (handles "../" prefixes). | ||
| const targetTail = target.replace(/^(\.\.\/)+/, ""); | ||
| const byTail = files.find((f) => { | ||
| const fp = normalize(f.path); | ||
| return fp === targetTail || fp.endsWith(`/${targetTail}`); | ||
| }); | ||
| if (byTail) return byTail.path; |
Contributor
There was a problem hiding this comment.
Avoid picking an arbitrary file for ambiguous relative links.
The trailing-segment fallback returns the first match, so links like ../README.md can open the wrong file when multiple folders contain README.md. Match only when the suffix is unique, like the bare-filename fallback already does.
Proposed fix
- const byTail = files.find((f) => {
+ const tailMatches = files.filter((f) => {
const fp = normalize(f.path);
return fp === targetTail || fp.endsWith(`/${targetTail}`);
});
- if (byTail) return byTail.path;
+ if (tailMatches.length === 1) return tailMatches[0].path;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fall back to matching by trailing path segments (handles "../" prefixes). | |
| const targetTail = target.replace(/^(\.\.\/)+/, ""); | |
| const byTail = files.find((f) => { | |
| const fp = normalize(f.path); | |
| return fp === targetTail || fp.endsWith(`/${targetTail}`); | |
| }); | |
| if (byTail) return byTail.path; | |
| // Fall back to matching by trailing path segments (handles "../" prefixes). | |
| const targetTail = target.replace(/^(\.\.\/)+/, ""); | |
| const tailMatches = files.filter((f) => { | |
| const fp = normalize(f.path); | |
| return fp === targetTail || fp.endsWith(`/${targetTail}`); | |
| }); | |
| if (tailMatches.length === 1) return tailMatches[0].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 `@ui/app/workspace/skills-repo/components/shared.tsx` around lines 370 - 376,
The byTail fallback logic in the relative link resolution uses files.find()
which returns the first arbitrary match when multiple files share the same
trailing path segment, potentially opening the wrong file. Replace the find()
call with filter() to get all matching files and only return the file path if
exactly one unique match exists, ensuring the fallback only succeeds when the
trailing path segment is unambiguous, consistent with how the bare-filename
fallback operates.
Comment on lines
+180
to
+182
| {!isFetching && accumulated.length === 0 && ( | ||
| <CommandEmpty>{debouncedSearch ? "No versions match your search" : "No versions yet"}</CommandEmpty> | ||
| )} |
Contributor
There was a problem hiding this comment.
Avoid rendering empty-state and error-state together.
When isError is true and there are no items, both messages can render at once (CommandEmpty and “Failed to load versions”). Gate the empty-state on !isError to avoid contradictory feedback.
Suggested fix
- { !isFetching && accumulated.length === 0 && (
+ { !isFetching && !isError && accumulated.length === 0 && (
<CommandEmpty>{debouncedSearch ? "No versions match your search" : "No versions yet"}</CommandEmpty>
)}Also applies to: 210-212
🤖 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 `@ui/app/workspace/skills-repo/dialogs/skillVersionDialog.tsx` around lines 180
- 182, The CommandEmpty component is rendering even when isError is true,
causing both the empty-state message and error-state message to display
simultaneously. Add !isError to the condition that checks !isFetching &&
accumulated.length === 0 for the CommandEmpty rendering to prevent the
empty-state from appearing when an error has occurred. Apply this same fix to
the other similar condition also mentioned at lines 210-212.
Comment on lines
+284
to
+289
| <Input | ||
| placeholder="Search files..." | ||
| value={fileSearchQuery} | ||
| onChange={(e) => setFileSearchQuery(e.target.value)} | ||
| data-testid="sidebar-search" | ||
| className="h-9 border-none pl-8" |
Contributor
There was a problem hiding this comment.
Keep new skill editor test IDs complete and consistent.
The sidebar search test ID does not follow the 3-part pattern, and the new version popover Cancel button has no data-testid. As per coding guidelines, “E2E tests must use data-testid attributes for element selection; convention is data-testid="<entity>-<element>-<qualifier>".” Based on learnings, “Enforce the 3-part data-testid convention.”
Proposed fix
- data-testid="sidebar-search"
+ data-testid="skill-file-search-input"
...
- <Button variant="ghost" size="sm" onClick={onClose}>
+ <Button variant="ghost" size="sm" data-testid="skill-version-cancel-btn" onClick={onClose}>Also applies to: 813-815
🤖 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 `@ui/app/workspace/skills-repo/forms/skillEditForm.tsx` around lines 284 - 289,
Update the data-testid attributes to follow the 3-part convention pattern
(entity-element-qualifier). The Input element with placeholder "Search files..."
currently has data-testid="sidebar-search" which is only 2 parts; update it to
follow the pattern (e.g., "sidebar-search-input"). Additionally, locate the new
version popover Cancel button around lines 813-815 and add a missing data-testid
attribute following the same 3-part convention pattern (e.g.,
"popover-version-cancel" or similar entity-element-qualifier format).
Sources: Coding guidelines, Learnings
Comment on lines
+294
to
+304
| <FileManagerSection | ||
| files={form.files} | ||
| onAddFile={form.addFile} | ||
| onRemoveFile={form.removeFile} | ||
| onUpdateFile={form.updateFile} | ||
| readOnly={false} | ||
| selectedIndex={selectedDetailsPane == null ? selectedFileIndex : null} | ||
| onSelectFile={(index) => { | ||
| setSelectedDetailsPane(null); | ||
| setSelectedFileIndex(index); | ||
| }} |
Contributor
There was a problem hiding this comment.
Adjust the selected file index when files are removed.
The parent tracks selection by array index, but onRemoveFile={form.removeFile} does not clear/decrement selectedFileIndex. Deleting the selected file can silently show the next file; deleting an earlier file can lose the current selection.
Proposed fix
<FileManagerSection
files={form.files}
onAddFile={form.addFile}
- onRemoveFile={form.removeFile}
+ onRemoveFile={(index) => {
+ form.removeFile(index);
+ setSelectedFileIndex((current) => {
+ if (current == null) return current;
+ if (current === index) return null;
+ return index < current ? current - 1 : current;
+ });
+ }}
onUpdateFile={form.updateFile}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <FileManagerSection | |
| files={form.files} | |
| onAddFile={form.addFile} | |
| onRemoveFile={form.removeFile} | |
| onUpdateFile={form.updateFile} | |
| readOnly={false} | |
| selectedIndex={selectedDetailsPane == null ? selectedFileIndex : null} | |
| onSelectFile={(index) => { | |
| setSelectedDetailsPane(null); | |
| setSelectedFileIndex(index); | |
| }} | |
| <FileManagerSection | |
| files={form.files} | |
| onAddFile={form.addFile} | |
| onRemoveFile={(index) => { | |
| form.removeFile(index); | |
| setSelectedFileIndex((current) => { | |
| if (current == null) return current; | |
| if (current === index) return null; | |
| return index < current ? current - 1 : current; | |
| }); | |
| }} | |
| onUpdateFile={form.updateFile} | |
| readOnly={false} | |
| selectedIndex={selectedDetailsPane == null ? selectedFileIndex : null} | |
| onSelectFile={(index) => { | |
| setSelectedDetailsPane(null); | |
| setSelectedFileIndex(index); | |
| }} |
🤖 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 `@ui/app/workspace/skills-repo/forms/skillEditForm.tsx` around lines 294 - 304,
The FileManagerSection component is using selectedFileIndex to track which file
is selected, but when onRemoveFile is called (which invokes form.removeFile),
the selectedFileIndex is not being updated. This causes the selection to become
misaligned: deleting the selected file silently switches to another file at the
same index, and deleting an earlier file loses the current selection due to
index shifting. Add logic to the onRemoveFile handler that adjusts
selectedFileIndex appropriately - either by clearing it when the selected file
is removed, or by decrementing it when a file earlier in the array is removed.
Comment on lines
+446
to
+449
| {/* Dims the page behind the version popover so it reads as a focused step, like a dialog. */} | ||
| {versionPopover != null && ( | ||
| <div className="animate-in fade-in-0 fixed inset-0 z-50 bg-black/50" onClick={closeVersionPopover} aria-hidden /> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "skillEditForm.tsx" | head -5Repository: maximhq/bifrost
Length of output: 114
🏁 Script executed:
wc -l ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 116
🏁 Script executed:
sed -n '440,530p' ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 3507
🏁 Script executed:
sed -n '520,530p' ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 585
🏁 Script executed:
find ui -type f -name "*popover*" -o -name "*Popover*" | head -10Repository: maximhq/bifrost
Length of output: 144
🏁 Script executed:
rg "from.*popover|from.*Popover" ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 141
🏁 Script executed:
cat ui/components/ui/popover.tsxRepository: maximhq/bifrost
Length of output: 3049
🏁 Script executed:
head -30 ui/app/workspace/skills-repo/forms/skillEditForm.tsx | grep -E "^import"Repository: maximhq/bifrost
Length of output: 1582
🏁 Script executed:
sed -n '1,100p' ui/app/workspace/skills-repo/forms/skillEditForm.tsx | grep -A5 "versionPopover\|openVersionPopover"Repository: maximhq/bifrost
Length of output: 601
🏁 Script executed:
rg "modal\|inert\|focus.*trap" ui/components/ui/popover.tsxRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
web_search
Radix UI react-popover modal focus trappingRepository: maximhq/bifrost
Length of output: 154
🌐 Web query:
Radix UI react-popover modal prop focus trap keyboard navigation
💡 Result:
In Radix UI, the Popover component includes a modal prop, but its implementation regarding focus management and interaction is specific [1][2]. The modal prop is intended to control the modality of the popover: when set to true, interaction with elements outside the popover is disabled, and the popover content is treated as modal for screen readers [1][2]. Regarding focus trapping and keyboard navigation: 1. Default Behavior: The Radix UI Popover is designed to provide accessible focus management. When opened, it typically traps focus within the popover content to ensure users remain within the expected context [3]. 2. Limitations of the modal prop: Users have reported that changing the modal prop does not always result in a complete removal of focus trapping behavior [3]. Specifically, the library has historically used a focus loop pattern to keep keyboard navigation contained within the portal [3]. 3. Keyboard Interaction: The standard keyboard navigation for the Popover includes: - Space/Enter: Opens or closes the popover [4][1]. - Tab/Shift + Tab: Moves focus between focusable elements within the popover [4][1]. - Esc: Closes the popover and returns focus to the trigger element [4][1]. If you require non-modal behavior where focus is not strictly trapped and can freely move to elements outside the popover, you may find that the default internal focus management logic remains active [3]. Developers seeking to override default focus behaviors often utilize the provided event handlers—such as onOpenAutoFocus and onCloseAutoFocus—to prevent default focus movements or manually control where focus is placed [1][5]. For complex menu-like interactions where specific focus ordering is required, Radix UI maintainers often suggest evaluating other primitives like DropdownMenu, which are designed with different focus management paradigms tailored to those specific use cases [6].
Citations:
- 1: https://www.mintlify.com/radix-ui/primitives/components/popover
- 2: https://radix-ui-primitives.mintlify.app/components/popover
- 3: 'modal' prop on Popover has no effect. Focus is always trapped. radix-ui/primitives#1338
- 4: https://www.radix-ui.com/primitives/docs/components/popover.md
- 5: https://github.com/radix-ui/website/blob/main/data/primitives/docs/components/popover.mdx
- 6: Focus order in Popover radix-ui/primitives#1922
🏁 Script executed:
rg "modal\|inert" ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
grep -n "PopoverContent\|Popover" ui/app/workspace/skills-repo/forms/skillEditForm.tsx | grep -E "(align|modal|prop)" | head -20Repository: maximhq/bifrost
Length of output: 232
🏁 Script executed:
rg "PopoverPrimitive.Root|Popover\s*\{" ui/components/ui/popover.tsx -A2Repository: maximhq/bifrost
Length of output: 213
🏁 Script executed:
sed -n '460,480p' ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 1018
🏁 Script executed:
rg "focus|trap|inert|modal" ui/ -t tsx | grep -i "popover\|overlay" | head -10Repository: maximhq/bifrost
Length of output: 87
🏁 Script executed:
sed -n '460,535p' ui/app/workspace/skills-repo/forms/skillEditForm.tsx | grep -E "Popover|PopoverContent|PopoverAnchor" -B2 -A8Repository: maximhq/bifrost
Length of output: 2753
🏁 Script executed:
rg "VersionPopoverBody" ui/app/workspace/skills-repo/forms/skillEditForm.tsx -B2 -A10 | head -50Repository: maximhq/bifrost
Length of output: 1410
🏁 Script executed:
find ui -type f -name "*VersionPopover*" -o -name "*versionPopover*"Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
sed -n '460,480p' ui/app/workspace/skills-repo/forms/skillEditForm.tsx | cat -nRepository: maximhq/bifrost
Length of output: 1165
🏁 Script executed:
rg "modal|onOpenAutoFocus|onCloseAutoFocus" ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
cat ui/components/ui/popover.tsx | grep -E "Root|modal|focus" -iRepository: maximhq/bifrost
Length of output: 299
🏁 Script executed:
git log --oneline -20Repository: maximhq/bifrost
Length of output: 1270
🏁 Script executed:
git diff HEAD~5..HEAD --stat | head -30Repository: maximhq/bifrost
Length of output: 1514
🏁 Script executed:
rg "PopoverPrimitive.Root" -A5 ui/components/ui/popover.tsxRepository: maximhq/bifrost
Length of output: 387
🏁 Script executed:
cat ui/components/ui/popover.tsxRepository: maximhq/bifrost
Length of output: 3049
🏁 Script executed:
sed -n '460,540p' ui/app/workspace/skills-repo/forms/skillEditForm.tsx | grep -E "Popover|modal|focus"Repository: maximhq/bifrost
Length of output: 1476
🏁 Script executed:
rg "modal\s*=" ui/app/workspace/skills-repo/forms/skillEditForm.tsxRepository: maximhq/bifrost
Length of output: 41
Add modal={true} to Popover components or implement explicit focus trapping for keyboard users.
The custom overlay dims interactions but does not trap focus; keyboard users can Tab into controls behind the popover. Either pass modal={true} to the three Popover Root components (lines 466, 490, 518) to enable Radix UI's built-in focus management, or use explicit focus-trap utilities and ensure Tab/Shift+Tab remain within the popover content.
🤖 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 `@ui/app/workspace/skills-repo/forms/skillEditForm.tsx` around lines 446 - 449,
The custom overlay dims the page but does not trap keyboard focus, allowing
users to Tab into controls behind the popover. Add the `modal={true}` prop to
all three Popover Root components (located at lines 466, 490, and 518) to enable
Radix UI's built-in focus management. This will automatically trap focus within
the popover when it is open, ensuring keyboard users cannot navigate to elements
behind the overlay.
Comment on lines
+188
to
190
| // Relative periods are durations (last hour, last 7 days) and are | ||
| // timezone-independent, so we intentionally omit the timezone badge here. | ||
| <span>{props.preDefinedPeriods?.find((p) => p.value === predefinedPeriod)?.label}</span> |
Contributor
There was a problem hiding this comment.
Timezone changes currently convert relative periods into fixed absolute ranges.
At Line 188-Line 190, relative periods are explicitly treated as timezone-independent, but Line 320-Line 344 still re-emits onDateTimeUpdate on timezone change. In current consumers, that clears the predefined period and silently switches the filter mode.
Suggested fix
onValueChange={(v) => {
if (!v) return;
props.onTimezoneChange?.(v);
+ // Relative periods are timezone-independent; keep them as relative filters.
+ if (predefinedPeriod) return;
// Wall-clock stays fixed: re-emit the same calendar day + time
// interpreted in the newly selected zone so the query shifts.
if (date?.from && date?.to) {
const fromInActiveTimezone = activeTimezone ? new TZDate(date.from.getTime(), activeTimezone) : date.from;
const toInActiveTimezone = activeTimezone ? new TZDate(date.to.getTime(), activeTimezone) : date.to;
const from = new TZDate(Also applies to: 315-344
🤖 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 `@ui/components/ui/datePickerWithRange.tsx` around lines 188 - 190, The
timezone change handler in the 320-344 range is calling onDateTimeUpdate for all
date selections, including relative periods which are explicitly marked as
timezone-independent at lines 188-190. This causes relative periods to be
silently converted to absolute ranges. Add a check in the timezone change
handler to detect whether the current selection is a predefined/relative period
by checking if predefinedPeriod is set, and skip the onDateTimeUpdate call for
relative periods since they don't require timezone adjustments. Only emit the
update for absolute date range selections.
| data-slot="resizable-handle" | ||
| className={cn( | ||
| "bg-border focus-visible:ring-ring group/resizable-handle relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:ring-1 focus-visible:ring-offset-1 focus-visible:outline-hidden data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:translate-x-0 data-[panel-group-direction=vertical]:after:-translate-y-1/2 [&[data-panel-group-direction=vertical]>div]:rotate-90", | ||
| "bg-border focus-visible:ring-ring group/resizable-handle relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:ring-offset-1 focus-visible:outline-hidden data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:translate-x-0 data-[panel-group-direction=vertical]:after:-translate-y-1/2 [&[data-panel-group-direction=vertical]>div]:rotate-90 focus-visible:ring-0", |
Contributor
There was a problem hiding this comment.
Restore a visible focus indicator on the resizable handle.
focus-visible:ring-0 cancels the ring while focus-visible:outline-hidden also hides the outline, so keyboard users lose the focus cue for this interactive separator. Restore the previous visible ring or add an equivalent focus style.
Proposed fix
- "bg-border focus-visible:ring-ring group/resizable-handle relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:ring-offset-1 focus-visible:outline-hidden data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:translate-x-0 data-[panel-group-direction=vertical]:after:-translate-y-1/2 [&[data-panel-group-direction=vertical]>div]:rotate-90 focus-visible:ring-0",
+ "bg-border focus-visible:ring-ring group/resizable-handle relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:ring-1 focus-visible:ring-offset-1 focus-visible:outline-hidden data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:translate-x-0 data-[panel-group-direction=vertical]:after:-translate-y-1/2 [&[data-panel-group-direction=vertical]>div]:rotate-90",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "bg-border focus-visible:ring-ring group/resizable-handle relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:ring-offset-1 focus-visible:outline-hidden data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:translate-x-0 data-[panel-group-direction=vertical]:after:-translate-y-1/2 [&[data-panel-group-direction=vertical]>div]:rotate-90 focus-visible:ring-0", | |
| "bg-border focus-visible:ring-ring group/resizable-handle relative flex w-px items-center justify-center after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2 focus-visible:ring-1 focus-visible:ring-offset-1 focus-visible:outline-hidden data-[panel-group-direction=vertical]:h-px data-[panel-group-direction=vertical]:w-full data-[panel-group-direction=vertical]:after:left-0 data-[panel-group-direction=vertical]:after:h-1 data-[panel-group-direction=vertical]:after:w-full data-[panel-group-direction=vertical]:after:translate-x-0 data-[panel-group-direction=vertical]:after:-translate-y-1/2 [&[data-panel-group-direction=vertical]>div]:rotate-90", |
🤖 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 `@ui/components/ui/resizable.tsx` at line 39, The className string for the
resizable handle contains focus-visible:ring-0 which removes the focus ring and
focus-visible:outline-hidden which hides the outline, making the interactive
separator invisible to keyboard users when focused. Replace focus-visible:ring-0
with focus-visible:ring-2 (or similar appropriate ring class) to restore the
visible focus indicator, and remove or replace focus-visible:outline-hidden with
an appropriate focus styling class that maintains visual feedback for keyboard
navigation.
) ## Summary The `SkipKeySelection` flag (used for Claude Code OAuth/max mode) was previously bypassing governance entirely when no virtual key was present. This meant keyless OAuth users skipped header validation, VK mandatory checks, and all other governance enforcement. This PR removes that bypass so `SkipKeySelection` has no effect on governance outcomes — the flag's purpose of passing through OAuth tokens is handled independently in core key selection, not in the governance plugin. ## Changes - Removed the early-return bypass in `PreLLMHook` that skipped governance when `SkipKeySelection` was set and no virtual key was present - Moved required header validation before the virtual key extraction, ensuring it always runs regardless of key selection mode - Updated `TestPreLLMHook_SkipKeySelection` to reflect the new behavior: `SkipKeySelection` with no VK is now subject to `IsVkMandatory` like any other request, and `SkipKeySelection` with a VK is enforced identically to a non-skip request with the same VK - Updated test comments and assertion messages to accurately describe the expected behavior ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./plugins/governance/... ``` Expected: all `TestPreLLMHook_SkipKeySelection` subtests pass, confirming that: - `SkipKeySelection` + no VK is rejected when `IsVkMandatory=true` - `SkipKeySelection` + no VK is allowed when `IsVkMandatory=false` - `SkipKeySelection` + a VK is enforced the same as a non-skip request with the same VK ## Breaking changes - [x] Yes - [ ] No Keyless Claude Code OAuth/max mode requests will now be subject to governance enforcement (header validation, VK mandatory checks, etc.) rather than bypassing it. Deployments relying on the previous bypass behavior for keyless OAuth users will need to ensure `IsVkMandatory` is set to `false` if those users should be permitted without a virtual key. ## Related issues ## Security considerations This change closes a governance bypass path where requests with `SkipKeySelection` and no virtual key could circumvent all governance controls, including required header validation and virtual key enforcement policies. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] 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 new `evaluation_mode` configuration option for guardrail rules, allowing operators to control whether conversation turns are sent to guardrail providers as a single bundled call or evaluated individually per turn. This addresses false-positive blocks caused by context-sensitive classifiers (e.g., AWS Bedrock prompt-attack detection) incorrectly combining unrelated conversation turns. ## Changes - Added `evaluation_mode` field to guardrail rule configuration with two options: - `bundled` (default): concatenates all turns into a single guardrail provider call, preserving existing behavior - `per_turn`: sends each turn as an isolated guardrail call, preventing cross-turn context from triggering false-positive blocks at the cost of additional provider calls - Schema definitions updated in both the Bifrost Helm chart and the transports config schema - Example configuration added to `values.yaml` as a commented reference ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Configure a guardrail rule with `evaluation_mode: "per_turn"` and send a multi-turn conversation through a guardrail provider that uses context-sensitive classification (e.g., Bedrock prompt-attack). Verify that turns are evaluated in isolation and that previously observed cross-turn false positives no longer trigger a block. ```sh go test ./... ``` Set `evaluation_mode` in your guardrail rule config: ```yaml # "bundled" (default) | "per_turn" evaluation_mode: "per_turn" ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The `per_turn` mode increases the number of calls made to external guardrail providers per request. Operators should be aware of potential cost and rate-limit implications when enabling this mode at scale. ## 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
Fixes several small bugs and inconsistencies in the `litellm-to-bifrost` migration scripts, including a silent failure when required environment variables are set to whitespace-only values, a typo in a script comment, an incorrect migration command path in documentation, and a shell variable expansion issue in `seed_models.sh`.
## Changes
- `requireEnv` now trims whitespace from the environment variable value and treats a blank (whitespace-only) value as missing, preventing silent failures when variables are set but empty or contain only spaces
- Fixed a double-underscore typo (`delete_bifrost__entities.sh`) in the `delete_bifrost_entities.sh` usage comment
- Corrected the migration command path in `delete_entities.sh` from `go run ./scripts/migration/litellm` to `go run ./scripts/litellm-to-bifrost`
- Replaced the inline `${DRY_RUN:+(dry-run) }` expansion in `seed_models.sh` with an explicit conditional assignment to avoid unexpected behavior when `DRY_RUN` is set to a value other than `1`
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
Run the migration script with a whitespace-only required environment variable to confirm it now exits with a fatal error instead of proceeding silently:
```sh
export BIFROST_API_KEY=" "
go run ./scripts/litellm-to-bifrost
# Expected: fatal log message indicating BIFROST_API_KEY is required
```
Run `seed_models.sh` with `DRY_RUN=1` and verify the output includes `(dry-run)`:
```sh
DRY_RUN=1 ./scripts/litellm-to-bifrost/scripts/seed_models.sh
# Expected: "Seeding complete: N model(s) (dry-run) processed, 0 failed."
```
## Breaking changes
- [ ] Yes
- [x] No
## 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 Adds a migration guide for moving from LiteLLM to Bifrost using the `@maximhq/bifrost-migration-cli` npx tool. The guide covers how the tool reads LiteLLM configuration, credentials, and governance entities and recreates them in Bifrost through the management API. ## Changes - Added `docs/migration-guides/litellm.mdx` with full documentation for the LiteLLM-to-Bifrost migration tool, including configuration environment variables, credential resolution from `config.yaml` and Postgres, entity mapping tables (models, organizations, teams, users, virtual keys), dry-run and idempotency behavior, provider name normalization, provider compatibility, and troubleshooting guidance. - Registered the new page in `docs/docs.json` under the Migration Guides section. ## 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 Navigate to the docs site and verify the new "Migrating from LiteLLM" page appears under Migration Guides and renders correctly, including all tables, steps, notes, warnings, and code blocks. ## Breaking changes - [ ] Yes - [x] No ## 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 Renames the `scripts/litellm-to-bifrost` directory to `scripts/bifrost-migration-cli` to better reflect that the tool is a general-purpose Bifrost migration CLI rather than being scoped solely to LiteLLM. This also marks the initial versioned release (`0.1.0`) of the CLI. ## Changes - Renamed `scripts/litellm-to-bifrost` to `scripts/bifrost-migration-cli` across all files and import paths. - Added a `version` file pinned at `0.1.0` and a `changelog.md` noting the initial release. - Added a `version` subcommand to the CLI binary that prints the version and commit hash at runtime (injected via build flags). - Added a `.gitignore` to exclude the compiled `bifrost-migration-cli` binary. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd scripts/bifrost-migration-cli go build -ldflags "-X main.version=$(cat version) -X main.commit=$(git rev-parse --short HEAD)" -o bifrost-migration-cli . ./bifrost-migration-cli version # Expected: bifrost-migration-cli 0.1.0 (<short-commit>) ``` ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No Any existing scripts or tooling that reference `scripts/litellm-to-bifrost` or the Go module path `github.com/maximhq/bifrost/scripts/litellm-to-bifrost` will need to be updated to use `scripts/bifrost-migration-cli` and `github.com/maximhq/bifrost/scripts/bifrost-migration-cli` respectively. ## Related issues N/A ## Security considerations None. ## 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 Introduces a new `npx`\-runnable package (`@maximhq/bifrost-migration-cli`) that acts as a thin Node.js wrapper for downloading and executing the `bifrost-migration-cli` binary. This allows users to run the migration tool directly via `npx @maximhq/bifrost-migration-cli` without manually downloading or installing the binary. ## Changes - Added `npx/bifrost-migration-cli/bin.js`: A Node.js script that detects the host platform and architecture, downloads the appropriate pre-built `bifrost-migration-cli` binary from `https://downloads.getmaxim.ai`, installs it to `~/.bifrost/bin/`, and executes it with any forwarded arguments. - Added `npx/bifrost-migration-cli/package.json`: Defines the `@maximhq/bifrost-migration-cli` npm package, wiring `bin.js` as the CLI entry point. - The binary is always re-downloaded on each invocation to ensure `latest` never serves a stale cached release. - Downloads are written to a unique temp file (`<binary>.download-<pid>-<timestamp>`) and atomically renamed into place only after SHA-256 checksum verification passes. The temp file is cleaned up on any failure. - Supports a `--cli-version` flag (e.g., `--cli-version=v1.2.3`) to pin a specific binary release; defaults to `latest`. - Validates version format and checks for binary existence on the download server before attempting a download, providing clear error messages on failure. - Enforces request timeouts (30s for HEAD/checksum requests, 300s for binary downloads) and surfaces timeout errors with descriptive messages. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh # Run with latest binary npx @maximhq/bifrost-migration-cli --help # Run with a specific version npx @maximhq/bifrost-migration-cli --cli-version=v1.0.0 --help # Verify checksum enforcement: tamper with the downloaded binary and confirm rejection # Verify invalid versions produce a descriptive error npx @maximhq/bifrost-migration-cli --cli-version=v0.0.0-nonexistent --help ``` Expected: binary is downloaded and checksum-verified on each run. Invalid or nonexistent versions should produce a descriptive error and exit with a non-zero code. A tampered binary should be deleted and the process should exit with a checksum failure message. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations - The binary is verified against a SHA-256 checksum fetched from the download server before execution. If the checksum file is unavailable or the hash does not match, the downloaded binary is deleted and the process exits. - The binary is written with `0o755` permissions and executed directly; ensure the download endpoint (`https://downloads.getmaxim.ai`) is trusted and access-controlled. ## 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 Adds an automated CI/CD pipeline to build, test, and release the `bifrost-migration-cli` tool. When a push to `main` is detected and the version file contains a version that hasn't been tagged yet, the workflow runs tests, cross-compiles binaries for multiple platforms, uploads them to Cloudflare R2, and creates a GitHub release with a changelog-based release body. ## Changes - Added a GitHub Actions workflow (`release-bifrost-migration-cli.yml`) that triggers on pushes to `main`, checks whether the current version has already been released by inspecting existing git tags, runs tests, and conditionally proceeds to release. - Added `build-bifrost-migration-cli-executables.sh` to cross-compile the CLI for `darwin/amd64`, `darwin/arm64`, `linux/amd64`, `linux/arm64`, and `windows/amd64` with trimmed binaries, embedded version/commit metadata, and SHA-256 checksums per binary. - Added `release-bifrost-migration-cli.sh` to orchestrate the full release: build validation, changelog extraction and deduplication against the previous tag, R2 upload, git tagging, and GitHub release creation. Prerelease versions (those containing a hyphen) are flagged accordingly and skipped from the `latest/` R2 path. - Added `upload-bifrost-migration-cli-to-r2.sh` to handle uploading build artifacts to R2 with retry logic (exponential backoff, up to 3 attempts). Stable releases are also mirrored to a `latest/` path; prereleases are not. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Bump the version in `scripts/bifrost-migration-cli/version` to a value that does not yet have a corresponding git tag, then push to `main`. The workflow should: 1. Detect the new version. 2. Run `go test ./...` inside `scripts/bifrost-migration-cli`. 3. Build binaries for all five target platforms. 4. Upload artifacts to R2 under the versioned path (and `latest/` for stable releases). 5. Create a git tag `bifrost-migration-cli/vX.Y.Z` and a corresponding GitHub release. To validate locally: ```sh cd scripts/bifrost-migration-cli GOWORK=off go test ./... # Build executables manually bash .github/workflows/scripts/build-bifrost-migration-cli-executables.sh <version> ls dist/ ``` Required secrets: `GH_TOKEN`, `R2_ENDPOINT`, `R2_ACCESS_KEY_ID`, `R2_SECRET_ACCESS_KEY`, `R2_BUCKET`. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations - The `check-version` job uses `egress-policy: block` with only `github.com:443` allowed, minimizing the attack surface during tag inspection. - Release and test jobs use `egress-policy: audit` to log outbound calls. - `GH_TOKEN` and R2 credentials are consumed exclusively via GitHub Actions secrets and are never echoed to logs. - All third-party actions are pinned to full commit SHAs. ## 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm-charts/bifrost/values.schema.json (1)
1043-1046:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit schema default for
group_traces_by_session.The field is documented as defaulting to
false, but the property does not currently declare"default": false, which weakens schema/UI parity for this new config.🧩 Suggested patch
"group_traces_by_session": { "type": "boolean", + "default": false, "description": "Group requests sharing the same x-bf-session-id header into a single APM trace, with each request as a top-level sibling span. Agent mode only; an inbound W3C traceparent takes precedence (default: false)" },As per coding guidelines,
transports/config.schema.jsonis the source of truth for config fields.🤖 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 `@helm-charts/bifrost/values.schema.json` around lines 1043 - 1046, The group_traces_by_session property in the schema is missing an explicit default value declaration even though its description documents the default as false. Add a "default": false property to the group_traces_by_session field definition in the values.schema.json file to ensure schema and UI parity, maintaining consistency with the documented behavior and the source of truth in transports/config.schema.json.Source: Coding guidelines
🧹 Nitpick comments (1)
.github/workflows/scripts/upload-bifrost-migration-cli-to-r2.sh (1)
51-51: 💤 Low valueQuote variable to prevent word splitting.
ShellCheck flagged this line. While
$attemptis numeric and unlikely to cause issues, quoting defensive coding is a best practice.🔧 Suggested fix
- if [ $attempt -lt $max_retries ]; then + if [ "$attempt" -lt "$max_retries" ]; then🤖 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 @.github/workflows/scripts/upload-bifrost-migration-cli-to-r2.sh at line 51, The variable $attempt in the conditional comparison needs to be quoted to prevent word splitting and follow shell scripting best practices. In the condition where $attempt is compared to $max_retries using the -lt operator, wrap $attempt with double quotes to make it "$attempt" while keeping the rest of the comparison unchanged.Source: Linters/SAST tools
🤖 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 @.github/workflows/release-bifrost-migration-cli.yml:
- Around line 64-68: Add the `persist-credentials: false` parameter to the
checkout step that uses the actions/checkout action to reduce the attack
surface. Since this workflow only requires read access to the repository and
does not perform any git write operations like pushing commits or tags,
explicitly disabling credential persistence is a security best practice. Include
this parameter in the `with` section alongside the existing `fetch-depth` and
`fetch-tags` parameters.
In @.github/workflows/scripts/release-bifrost-migration-cli.sh:
- Around line 92-98: The tag existence check in the conditional block only
verifies local refs using git rev-parse, which means a tag could exist locally
but not be pushed to the remote, causing the push to be skipped and making the
process non-idempotent. Modify the tag existence check to verify that the tag
exists on the remote repository using git ls-remote to check the origin remote
before deciding to skip the tag creation and push operations, ensuring
consistency between local and remote state.
In `@helm-charts/bifrost/templates/_helpers.tpl`:
- Line 690: The evaluation_mode field at line 690 is only set when explicitly
provided in the input, which means it will be omitted from the rendered output
when unset since Helm schema defaults are not injected at render-time. Modify
the conditional assignment to always set evaluation_mode by using either the
provided value or an explicit default value sourced from the
transports/config.schema.json schema definition. Replace the current
if-conditional block that only executes when .evaluation_mode is truthy with
logic that ensures the field is always included in the rendered output with a
sensible default.
---
Outside diff comments:
In `@helm-charts/bifrost/values.schema.json`:
- Around line 1043-1046: The group_traces_by_session property in the schema is
missing an explicit default value declaration even though its description
documents the default as false. Add a "default": false property to the
group_traces_by_session field definition in the values.schema.json file to
ensure schema and UI parity, maintaining consistency with the documented
behavior and the source of truth in transports/config.schema.json.
---
Nitpick comments:
In @.github/workflows/scripts/upload-bifrost-migration-cli-to-r2.sh:
- Line 51: The variable $attempt in the conditional comparison needs to be
quoted to prevent word splitting and follow shell scripting best practices. In
the condition where $attempt is compared to $max_retries using the -lt operator,
wrap $attempt with double quotes to make it "$attempt" while keeping the rest of
the comparison unchanged.
🪄 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: 4b86af76-fa54-4b4b-ac54-16b48caccde0
⛔ Files ignored due to path filters (1)
scripts/bifrost-migration-cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
.github/workflows/release-bifrost-migration-cli.yml.github/workflows/scripts/build-bifrost-migration-cli-executables.sh.github/workflows/scripts/release-bifrost-migration-cli.sh.github/workflows/scripts/upload-bifrost-migration-cli-to-r2.shdocs/docs.jsondocs/migration-guides/litellm.mdxhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlnpx/bifrost-migration-cli/bin.jsnpx/bifrost-migration-cli/package.jsonplugins/governance/main.goplugins/governance/modelprovidergovernance_test.goscripts/bifrost-migration-cli/.gitignorescripts/bifrost-migration-cli/bifrost.goscripts/bifrost-migration-cli/changelog.mdscripts/bifrost-migration-cli/go.modscripts/bifrost-migration-cli/litellm/crypto.goscripts/bifrost-migration-cli/litellm/db.goscripts/bifrost-migration-cli/litellm/litellm.goscripts/bifrost-migration-cli/litellm/models.goscripts/bifrost-migration-cli/litellm/organization.goscripts/bifrost-migration-cli/litellm/team.goscripts/bifrost-migration-cli/litellm/user.goscripts/bifrost-migration-cli/litellm/virtualkey.goscripts/bifrost-migration-cli/main.goscripts/bifrost-migration-cli/model.goscripts/bifrost-migration-cli/modelconformance_test.goscripts/bifrost-migration-cli/organization.goscripts/bifrost-migration-cli/scripts/create_organizations.shscripts/bifrost-migration-cli/scripts/create_teams.shscripts/bifrost-migration-cli/scripts/create_virtual_keys.shscripts/bifrost-migration-cli/scripts/delete_bifrost_entities.shscripts/bifrost-migration-cli/scripts/delete_entities.shscripts/bifrost-migration-cli/scripts/seed_models.shscripts/bifrost-migration-cli/team.goscripts/bifrost-migration-cli/user.goscripts/bifrost-migration-cli/utils.goscripts/bifrost-migration-cli/versionscripts/bifrost-migration-cli/virtualkey.gotransports/config.schema.json
💤 Files with no reviewable changes (9)
- scripts/bifrost-migration-cli/litellm/crypto.go
- scripts/bifrost-migration-cli/litellm/virtualkey.go
- scripts/bifrost-migration-cli/litellm/user.go
- scripts/bifrost-migration-cli/utils.go
- scripts/bifrost-migration-cli/litellm/organization.go
- scripts/bifrost-migration-cli/litellm/models.go
- scripts/bifrost-migration-cli/litellm/team.go
- scripts/bifrost-migration-cli/litellm/db.go
- scripts/bifrost-migration-cli/litellm/litellm.go
✅ Files skipped from review due to trivial changes (8)
- scripts/bifrost-migration-cli/.gitignore
- scripts/bifrost-migration-cli/version
- scripts/bifrost-migration-cli/changelog.md
- .github/workflows/scripts/build-bifrost-migration-cli-executables.sh
- npx/bifrost-migration-cli/package.json
- helm-charts/bifrost/values.yaml
- helm-charts/bifrost/README.md
- docs/migration-guides/litellm.mdx
Comment on lines
+64
to
+68
| - name: Checkout repository | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
| with: | ||
| fetch-depth: 0 | ||
| fetch-tags: true |
Contributor
There was a problem hiding this comment.
Add persist-credentials: false to the checkout step.
The static analysis tool flagged this checkout step for credential persistence. Since this job only needs read access and doesn't perform git operations, explicitly disabling credential persistence reduces the attack surface.
🛡️ Suggested fix
- name: Checkout repository
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
fetch-depth: 0
fetch-tags: true
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Checkout repository | |
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | |
| with: | |
| fetch-depth: 0 | |
| fetch-tags: true | |
| - name: Checkout repository | |
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | |
| with: | |
| fetch-depth: 0 | |
| fetch-tags: true | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 64-68: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/release-bifrost-migration-cli.yml around lines 64 - 68,
Add the `persist-credentials: false` parameter to the checkout step that uses
the actions/checkout action to reduce the attack surface. Since this workflow
only requires read access to the repository and does not perform any git write
operations like pushing commits or tags, explicitly disabling credential
persistence is a security best practice. Include this parameter in the `with`
section alongside the existing `fetch-depth` and `fetch-tags` parameters.
Source: Linters/SAST tools
Comment on lines
+92
to
+98
| if git rev-parse -q --verify "refs/tags/$TAG_NAME" >/dev/null; then | ||
| echo "ℹ️ Tag $TAG_NAME already exists. Reusing it." | ||
| else | ||
| echo "🏷️ Creating tag: $TAG_NAME" | ||
| git tag "$TAG_NAME" -m "Release bifrost-migration-cli v$VERSION" -m "$CHANGELOG_BODY" | ||
| git push origin "$TAG_NAME" | ||
| fi |
Contributor
There was a problem hiding this comment.
Check remote tag existence before skipping push.
At Line 92, the script only checks local refs. A local-only tag can skip git push, which makes the publish path non-idempotent and can break or mis-target release creation.
🔧 Suggested fix
-if git rev-parse -q --verify "refs/tags/$TAG_NAME" >/dev/null; then
- echo "ℹ️ Tag $TAG_NAME already exists. Reusing it."
-else
- echo "🏷️ Creating tag: $TAG_NAME"
- git tag "$TAG_NAME" -m "Release bifrost-migration-cli v$VERSION" -m "$CHANGELOG_BODY"
- git push origin "$TAG_NAME"
-fi
+if git ls-remote --exit-code --tags origin "refs/tags/$TAG_NAME" >/dev/null 2>&1; then
+ echo "ℹ️ Remote tag $TAG_NAME already exists. Reusing it."
+else
+ if git rev-parse -q --verify "refs/tags/$TAG_NAME" >/dev/null; then
+ echo "ℹ️ Local tag $TAG_NAME exists; pushing to origin."
+ else
+ echo "🏷️ Creating tag: $TAG_NAME"
+ git tag "$TAG_NAME" -m "Release bifrost-migration-cli v$VERSION" -m "$CHANGELOG_BODY"
+ fi
+ git push origin "refs/tags/$TAG_NAME"
+fi🤖 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 @.github/workflows/scripts/release-bifrost-migration-cli.sh around lines 92 -
98, The tag existence check in the conditional block only verifies local refs
using git rev-parse, which means a tag could exist locally but not be pushed to
the remote, causing the push to be skipped and making the process
non-idempotent. Modify the tag existence check to verify that the tag exists on
the remote repository using git ls-remote to check the origin remote before
deciding to skip the tag creation and push operations, ensuring consistency
between local and remote state.
| {{- if .sampling_rate }}{{- $_ := set $rule "sampling_rate" .sampling_rate }}{{- end }} | ||
| {{- if .timeout }}{{- $_ := set $rule "timeout" .timeout }}{{- end }} | ||
| {{- if hasKey . "max_turns_to_send" }}{{- $_ := set $rule "max_turns_to_send" .max_turns_to_send }}{{- end }} | ||
| {{- if .evaluation_mode }}{{- $_ := set $rule "evaluation_mode" .evaluation_mode }}{{- end }} |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Render evaluation_mode with an explicit default.
At Line 690, the field is omitted when unset. Helm schema defaults are not injected at render-time, so default parity is safer if set directly in template output.
♻️ Suggested change
-{{- if .evaluation_mode }}{{- $_ := set $rule "evaluation_mode" .evaluation_mode }}{{- end }}
+{{- $_ := set $rule "evaluation_mode" (default "bundled" .evaluation_mode) }}As per coding guidelines, transports/config.schema.json is the source of truth for config fields.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if .evaluation_mode }}{{- $_ := set $rule "evaluation_mode" .evaluation_mode }}{{- end }} | |
| {{- $_ := set $rule "evaluation_mode" (default "bundled" .evaluation_mode) }} |
🤖 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 `@helm-charts/bifrost/templates/_helpers.tpl` at line 690, The evaluation_mode
field at line 690 is only set when explicitly provided in the input, which means
it will be omitted from the rendered output when unset since Helm schema
defaults are not injected at render-time. Modify the conditional assignment to
always set evaluation_mode by using either the provided value or an explicit
default value sourced from the transports/config.schema.json schema definition.
Replace the current if-conditional block that only executes when
.evaluation_mode is truthy with logic that ensures the field is always included
in the rendered output with a sensible default.
Source: Coding guidelines
## Summary Adds tracing support for `business_unit` and `user` context attributes, enabling these governance-related identifiers to be captured in spans alongside the existing team and customer attributes. ## Changes - Added `AttrBifrostBusinessUnitID`, `AttrBifrostBusinessUnitName`, `AttrBifrostUserID`, and `AttrBifrostUserName` trace attribute constants to `core/schemas/trace.go` - Updated `PopulateContextAttributes` in `framework/tracing/llmspan.go` to accept and emit `businessUnitID`, `businessUnitName`, `userID`, and `userName` parameters when non-empty - Updated `executeRequestWithRetries` in `core/bifrost.go` to read `BifrostContextKeyGovernanceBusinessUnitID`, `BifrostContextKeyGovernanceBusinessUnitName`, `BifrostContextKeyUserID`, and `BifrostContextKeyUserName` from context and set them as span attributes ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Populate the relevant context keys (`BifrostContextKeyGovernanceBusinessUnitID`, `BifrostContextKeyGovernanceBusinessUnitName`, `BifrostContextKeyUserID`, `BifrostContextKeyUserName`) on a request context and verify the corresponding `bifrost.business_unit.id`, `bifrost.business_unit.name`, `bifrost.user.id`, and `bifrost.user.name` attributes appear on the resulting trace span. ```sh go test ./... ``` ## Breaking changes - [ ] Yes - [x] No ## Security considerations `userID` and `userName` values sourced from request context will be written to trace spans. Ensure that any user-identifying data emitted here complies with your data retention and PII policies for your tracing backend. ## 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 The `ContainerFileDelete` endpoint was missing a `Content-Type: application/json` header, which could cause request failures or unexpected behavior when calling the OpenAI containers API. ## Changes - Added `Content-Type: application/json` header to the `ContainerFileDelete` request to align with the expected request format for the OpenAI containers file deletion endpoint. ## 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 Send a `ContainerFileDelete` request and verify it completes successfully without header-related errors. ```sh make test-core PROVIDER=openai TESTCASE=TestOpenAI/OpenAITests/ContainerFileDelete make test-core PROVIDER=openai TESTCASE=TestOpenAI/OpenAITests/WithRawRequestResponse ``` ## Breaking changes - [ ] Yes - [x] No ## 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 Updates e2e tests for provider governance budgets and custom provider validation to align with recent UI changes, and fixes base URLs used in test fixtures to use real provider endpoints. ## Changes - Added `addBudgetLine()` and `getBudgetAmountInput()` helper methods to `ProvidersPage` to interact with the new budget line UI, replacing the old static `#providerBudgetMaxLimit` locator - Updated governance budget tests to call `addBudgetLine()` before interacting with the budget amount input, reflecting the new add-then-fill flow - Replaced placeholder/fake base URLs in custom provider test fixtures with real provider endpoints (`https://api.openai.com/v1`, `https://api.anthropic.com`) to avoid false failures from URL validation - Added a new test case that verifies an error toast is shown when a custom provider is saved with an invalid/non-resolvable hostname ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh cd tests/e2e pnpm test --grep "Providers|Governance" ``` Expected outcomes: - Custom provider creation tests pass with real base URLs - Governance budget tab tests correctly add a budget line before asserting input visibility - A new test confirms that submitting a custom provider with an invalid hostname (`https://api.nonexistent-provider.invalid/v1`) surfaces an "Invalid base URL" error toast and keeps the sheet open ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No security implications. Test fixtures now use real provider hostnames, but no real credentials are used. ## 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 Updates the model limits E2E page object to align with the current UI, where the model selector now defaults to "All Models" via a combobox rather than a searchable multiselect. Also introduces a reusable `setBudget` helper that handles adding a budget line, filling the amount, and optionally selecting a reset period. ## Changes - Replaced the model multiselect search-and-select flow with a single combobox click that selects "All Models", reflecting the current UI behavior. The selected model name is now hardcoded to `'*'`. - Extracted budget configuration into a private `setBudget` method used by both `createModelLimit` and `updateModelLimit`, replacing the previous inline `#modelBudgetMaxLimit` locator usage. - Added a `resetPeriodLabels` map to translate duration shorthand keys (e.g. `'1h'`, `'1d'`) into their human-readable dropdown labels (e.g. `'Hourly'`, `'Daily'`) for selecting reset periods in the budget line combobox. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the model limits E2E tests and verify that limit creation and update flows complete without errors, including budget lines with reset periods. ```sh cd tests/e2e pnpm test --grep "model-limits" ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. ## 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 Refactors the OAuth e2e test flow to use a direct API-based approach instead of relying on browser popup events. This resolves flakiness caused by waiting for popup windows to open and close during OAuth authorization in tests. ## Changes - Introduced a `completeOAuthFlow` helper that navigates to the authorization URL in a new page, completes the login form, then polls the OAuth status endpoint until `authorized` and calls the complete-oauth endpoint directly — bypassing the need to intercept popup events. - Added `createOAuthClient` method to `MCPRegistryPage` that intercepts the `pending_oauth` API response and returns the `authorize_url`, `oauth_config_id`, and related fields for use in `completeOAuthFlow`. - Updated `selectAuthType` to reflect the new UI split between auth type (e.g., `oauth`) and auth scope (e.g., `shared` vs `per_user`). `per_user_oauth` is now expressed as auth type `oauth` + scope `per_user`. - Added `selectAuthScope` method to handle the new `auth-scope-select` dropdown. - Added `expandOAuthAdvancedIfCollapsed` to expand the collapsible OAuth advanced section before interacting with fields like client ID and secret. - Updated OAuth input locators to prefer `data-testid` attributes with placeholder-based selectors as fallbacks. - Updated `viewClientDetails` to open the actions menu and click "Edit" rather than clicking the row directly. ## Type of change - [ ] Bug fix - [x] Refactor - [ ] Feature - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test Run the MCP registry e2e tests with the OAuth demo server running: ```sh MCP_SSE_HEADERS=1 npx playwright test tests/e2e/features/mcp-registry/mcp-registry.spec.ts ``` Verify that the OAuth and per-user OAuth client creation tests complete without flakiness, and that the created clients appear as connected in the registry table. ## Screenshots/Recordings N/A — no UI visual changes. ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No new auth flows or secrets handling introduced. The test helper uses the existing OAuth demo server and status/complete endpoints already present in the application. ## 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/features/mcp-registry/mcp-registry.spec.ts (1)
14-19: Replace the OAuth helper parameter type with Playwright'sPagetype for strict typing.The
completeOAuthFlowhelper at line 14 usesanytypes in its structural type hint. Replacepage: { context: () => any; request: any }withpage: Pageimported from@playwright/test. This eliminates theanyusage while maintaining compatibility with all four call sites (lines 423, 438, 454, 468).🤖 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/e2e/features/mcp-registry/mcp-registry.spec.ts` around lines 14 - 19, The completeOAuthFlow function parameter uses a structural type with any types instead of the proper Playwright Page type. First, import the Page type from `@playwright/test` at the top of the file. Then, replace the page parameter type definition from the structural type hint (page: { context: () => any; request: any }) with the proper Page type (page: Page) in the completeOAuthFlow function signature. This change will eliminate the any usage while maintaining full compatibility with all existing call sites.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.
Inline comments:
In `@tests/e2e/features/mcp-registry/pages/mcp-registry.page.ts`:
- Around line 410-415: The pendingOAuth detection uses brittle string matching
with responseBody.includes('"status":"pending_oauth"') which breaks if JSON
formatting changes. Instead, parse responseBody as JSON and compare the status
property directly to the string 'pending_oauth'. Wrap the JSON.parse() in a
try-catch block to safely handle cases where responseBody is empty or invalid
JSON, defaulting to false if parsing fails, so that the
waitForOAuthAuthorizerAction() call only executes when status is actually equal
to pending_oauth.
In `@tests/e2e/features/providers/pages/providers.page.ts`:
- Around line 684-690: The `aligned` configuration update is nested inside the
condition that checks for `config.budgets.length > 0`, which causes `aligned` to
be skipped when budgets array is empty or omitted. Extract the aligned
configuration logic outside of the budgets length check so that it can be
applied independently. The block handling the switchEl
(provider-governance-calendar-aligned-switch) and the isChecked attribute
comparison should execute whenever config.aligned is defined, regardless of
whether config.budgets is present or has items.
---
Nitpick comments:
In `@tests/e2e/features/mcp-registry/mcp-registry.spec.ts`:
- Around line 14-19: The completeOAuthFlow function parameter uses a structural
type with any types instead of the proper Playwright Page type. First, import
the Page type from `@playwright/test` at the top of the file. Then, replace the
page parameter type definition from the structural type hint (page: { context:
() => any; request: any }) with the proper Page type (page: Page) in the
completeOAuthFlow function signature. This change will eliminate the any usage
while maintaining full compatibility with all existing call sites.
🪄 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: 5c42d742-de3e-4891-887d-fba37fa45f5a
📒 Files selected for processing (6)
core/providers/openai/openai.gotests/e2e/features/mcp-registry/mcp-registry.spec.tstests/e2e/features/mcp-registry/pages/mcp-registry.page.tstests/e2e/features/model-limits/pages/model-limits.page.tstests/e2e/features/providers/pages/providers.page.tstests/e2e/features/providers/providers.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- core/providers/openai/openai.go
Comment on lines
+410
to
+415
| const responseBody = response ? await response.text().catch(() => '') : '' | ||
| const pendingOAuth = responseBody.includes('"status":"pending_oauth"') | ||
| if (pendingOAuth) { | ||
| await this.waitForOAuthAuthorizerAction() | ||
| return true | ||
| } |
Contributor
There was a problem hiding this comment.
Use JSON status parsing for pending_oauth detection.
responseBody.includes('"status":"pending_oauth"') is serialization-sensitive; a spacing/format change can skip the OAuth branch. Parse JSON and compare status directly.
Suggested fix
- const responseBody = response ? await response.text().catch(() => '') : ''
- const pendingOAuth = responseBody.includes('"status":"pending_oauth"')
+ const responseBody = response ? await response.json().catch(() => null) : null
+ const pendingOAuth = responseBody?.status === 'pending_oauth'🤖 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/e2e/features/mcp-registry/pages/mcp-registry.page.ts` around lines 410
- 415, The pendingOAuth detection uses brittle string matching with
responseBody.includes('"status":"pending_oauth"') which breaks if JSON
formatting changes. Instead, parse responseBody as JSON and compare the status
property directly to the string 'pending_oauth'. Wrap the JSON.parse() in a
try-catch block to safely handle cases where responseBody is empty or invalid
JSON, defaulting to false if parsing fails, so that the
waitForOAuthAuthorizerAction() call only executes when status is actually equal
to pending_oauth.
Comment on lines
+684
to
+690
| if (config.aligned !== undefined && config.budgets.length > 0) { | ||
| const switchEl = this.page.getByTestId('provider-governance-calendar-aligned-switch') | ||
| const isChecked = (await switchEl.getAttribute('data-state')) === 'checked' | ||
| if (isChecked !== config.aligned) { | ||
| await switchEl.click() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
aligned updates are skipped when budgets is omitted.
aligned is currently applied only inside the if (config.budgets) block, so { aligned: ... } alone does nothing despite being part of the public config shape.
🤖 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/e2e/features/providers/pages/providers.page.ts` around lines 684 - 690,
The `aligned` configuration update is nested inside the condition that checks
for `config.budgets.length > 0`, which causes `aligned` to be skipped when
budgets array is empty or omitted. Extract the aligned configuration logic
outside of the budgets length check so that it can be applied independently. The
block handling the switchEl (provider-governance-calendar-aligned-switch) and
the isChecked attribute comparison should execute whenever config.aligned is
defined, regardless of whether config.budgets is present or has items.
## Summary Adds a `test-cost-accuracy` CI job to the release pipeline that validates the correctness of LLM cost tracking end-to-end — from per-request log entries through aggregated stats, virtual key budget usage, and per-model quota breakdowns. ## Changes - Introduces a new `test-cost-accuracy` pipeline job that runs against any release-triggering component (core, framework, plugins, bifrost-http, or docker). - Adds `.github/workflows/scripts/cost-accuracy-test.sh`, which: - Spins up Postgres via the existing Docker Compose config and creates an isolated database. - Builds `bifrost-http`, `mocker`, and `hitter` binaries from source. - Writes a Bifrost config pointing at the mocker as the OpenAI provider backend. - Creates a virtual key with a budget and a scoped pricing override for `gpt-4o-mini` with configurable `INPUT_COST_PER_TOKEN` and `OUTPUT_COST_PER_TOKEN`. - Drives traffic through `hitter` at a configurable RPS and duration. - Validates that per-log costs, the `/api/logs/stats` total, virtual key budget `current_usage`, and per-model quota totals all match the expected value computed from token counts and the pricing override — failing with a detailed diff if any surface diverges by more than `1e-12`. - Wires `test-cost-accuracy` into the `needs` and gate conditions of all downstream release jobs (`core-release`, `framework-release`, `plugins-release`, `bifrost-http-release`, and all Docker publish jobs) so a cost accuracy failure blocks a release. - Uploads results and logs to a `cost-accuracy-results` artifact retained for 30 days. - Fixes a missing newline at the end of the workflow file. ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test The test runs automatically in CI on any branch that triggers a release. To run locally: ```sh export BENCHMARK_DIR=/path/to/bifrost-benchmarking # optional; cloned automatically if absent export COST_ACCURACY_RPS=10 export COST_ACCURACY_DURATION=10s export INPUT_COST_PER_TOKEN=0.000001 export OUTPUT_COST_PER_TOKEN=0.000002 chmod +x .github/workflows/scripts/cost-accuracy-test.sh .github/workflows/scripts/cost-accuracy-test.sh ``` Results are written to `tmp/cost-accuracy/results.json`. The script exits non-zero and prints a JSON diff if any cost surface mismatches. **Environment variables:** | Variable | Default | Description | |---|---|---| | `COST_ACCURACY_RPS` | `10` | Requests per second sent by hitter | | `COST_ACCURACY_DURATION` | `10s` | Duration of the load run | | `INPUT_COST_PER_TOKEN` | `0.000001` | Pricing override input rate | | `OUTPUT_COST_PER_TOKEN` | `0.000002` | Pricing override output rate | | `VIRTUAL_KEY_BUDGET_LIMIT` | `100` | Budget cap on the test virtual key | | `BENCHMARK_DIR` | `../bifrost-benchmarking` | Path to the benchmarking repo | ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations The job uses `egress-policy: block` with an explicit allowlist of endpoints. The mocker key and Postgres credentials are test-only values scoped to the ephemeral CI environment and are not persisted. ## 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
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/scripts/load-test.sh (1)
973-977: 💤 Low valueDeclare and assign separately to avoid masking
bcfailures.The combined declaration and assignment on these lines masks the exit code of the
echo ... | bcsubshell. Ifbcfails (e.g., due to malformed input from upstream extraction), the error would be silently ignored.- local us_mean=$(printf "%.2f" $(echo "scale=4; ($EXTRACTED_MEAN_NS - $CAL_MEAN_NS) / 1000" | bc)) + local us_mean + us_mean=$(printf "%.2f" $(echo "scale=4; ($EXTRACTED_MEAN_NS - $CAL_MEAN_NS) / 1000" | bc))Apply the same pattern to
us_50,us_90,us_95,us_99,success_pct, andactual_rpsdeclarations.Also applies to: 979-979, 989-989
🤖 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 @.github/workflows/scripts/load-test.sh around lines 973 - 977, The current code combines variable declaration and assignment on a single line, which masks any failures from the bc command. Separate the declaration from the assignment for each variable (us_mean, us_50, us_90, us_95, us_99, success_pct, and actual_rps). First declare each variable with the local keyword on its own line, then assign the result of the arithmetic operation on a separate line. This ensures that any errors from the bc subshell will be properly propagated instead of being silently ignored.Source: Linters/SAST tools
.github/workflows/scripts/cost-accuracy-test.sh (1)
302-446: 💤 Low valueFloating-point tolerance of 1e-12 may be too tight for cost comparisons.
The tolerance
1e-12used for comparing cost totals (lines 380, 405, 436-443) is extremely tight for floating-point arithmetic. With input/output costs like0.000001and0.000002, accumulated rounding errors across 100 requests could exceed this threshold depending on the order of operations and platform-specific floating-point behavior.Consider using a relative tolerance (e.g.,
1e-9or a percentage-based threshold) to avoid false failures in CI:-if math.fabs(actual - expected) > 1e-12: +if math.fabs(actual - expected) > 1e-9:🤖 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 @.github/workflows/scripts/cost-accuracy-test.sh around lines 302 - 446, The validate_costs function uses an absolute floating-point tolerance of 1e-12 for all cost comparisons, which is too strict and can cause false failures due to accumulated rounding errors in floating-point arithmetic. Replace all instances of the hardcoded 1e-12 tolerance value with a more reasonable tolerance such as 1e-9 or implement a relative tolerance approach. This applies to all math.fabs comparisons in the validate_costs function, including the per-log cost check, the budget comparison loop, and all final validation checks that compare expected_total, actual_total, stats_total, budget_current_usage_total, and quota_model_total against the expected values.
🤖 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 @.github/workflows/scripts/cost-accuracy-test.sh:
- Around line 302-446: The validate_costs function uses an absolute
floating-point tolerance of 1e-12 for all cost comparisons, which is too strict
and can cause false failures due to accumulated rounding errors in
floating-point arithmetic. Replace all instances of the hardcoded 1e-12
tolerance value with a more reasonable tolerance such as 1e-9 or implement a
relative tolerance approach. This applies to all math.fabs comparisons in the
validate_costs function, including the per-log cost check, the budget comparison
loop, and all final validation checks that compare expected_total, actual_total,
stats_total, budget_current_usage_total, and quota_model_total against the
expected values.
In @.github/workflows/scripts/load-test.sh:
- Around line 973-977: The current code combines variable declaration and
assignment on a single line, which masks any failures from the bc command.
Separate the declaration from the assignment for each variable (us_mean, us_50,
us_90, us_95, us_99, success_pct, and actual_rps). First declare each variable
with the local keyword on its own line, then assign the result of the arithmetic
operation on a separate line. This ensures that any errors from the bc subshell
will be properly propagated instead of being silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b409ae77-a813-417f-befb-0d2585dd2b6a
📒 Files selected for processing (5)
.github/workflows/release-pipeline.yml.github/workflows/scripts/cost-accuracy-test.sh.github/workflows/scripts/load-test-results.json.github/workflows/scripts/load-test-results.md.github/workflows/scripts/load-test.sh
💤 Files with no reviewable changes (2)
- .github/workflows/scripts/load-test-results.json
- .github/workflows/scripts/load-test-results.md
## Summary Bumps the indirect dependency `go.mongodb.org/mongo-driver` from `v1.17.6` to `v1.17.7` across all Go modules in the repository. ## Changes - Updated `go.mongodb.org/mongo-driver` from `v1.17.6` to `v1.17.7` in `framework`, `transports`, and all plugins (`compat`, `governance`, `logging`, `maxim`, `modelcatalogresolver`, `otel`, `semanticcache`, `telemetry`). ## 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 N/A ## Security considerations Patch-level dependency update to the MongoDB Go driver. No known security implications introduced by this change; upgrading to the latest patch may include upstream bug or security fixes. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] 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 MCP client connections are now deferred until after all plugins are registered, ensuring `PreMCPConnectionHook` runs against the complete plugin set. Previously, `NewMCPManager` dialed clients immediately during construction, which meant plugins registered after `Init` (e.g. enterprise plugins) were silently excluded from the hook and the client would only recover on a later health-monitor reconnect cycle. ## Changes - Extracted the parallel client-dialing logic from `NewMCPManager` into a new `ConnectConfiguredClients` method on `MCPManager`. Construction now only stores the boot configs; callers must explicitly invoke `ConnectConfiguredClients` when ready. - Added `ConnectConfiguredMCPClients` on `Bifrost` as the public entry point, delegating to `MCPManager.ConnectConfiguredClients` when MCP is configured. - Added `ConnectConfiguredClients` to `MCPManagerInterface` to keep the interface consistent. - In the HTTP server's `Bootstrap`, `ConnectConfiguredMCPClients` is called after all inference routes (and therefore all plugins) are registered. - Updated the MCP test fixture helper `setupMCPManager` to call `ConnectConfiguredClients` explicitly after construction. - Renamed the `logs_add_canonical_model_columns` migration to `logs_add_canonical_model_columns_v2` to fix a previously broken migration. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... go test ./core/internal/mcptests/... ``` 1. Configure one or more MCP clients in `MCPConfig.ClientConfigs` alongside at least one plugin that implements `PreMCPConnectionHook`. 2. Start the HTTP server and confirm the hook is invoked for each configured client during `Bootstrap` rather than during `Init`. 3. Simulate a connection failure for a boot client and verify the client is retained in `Disconnected` state and the health monitor recovers it automatically. ## Breaking changes - [x] Yes - [ ] No Callers that construct `MCPManager` directly via `NewMCPManager` must now call `manager.ConnectConfiguredClients(ctx)` explicitly after construction. The HTTP server transport handles this automatically. Any custom transport or embedding that relied on auto-connect during `NewMCPManager` will need to add this call. ## Related issues Closes #4556 (Anthropic duplicate `message_start` stream event, included in changelog) ## Security considerations No new auth surfaces or PII handling introduced. The change only affects the timing of MCP client connection establishment. ## 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
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
8 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.
No description provided.