guardrails changes #606
guardrails changes #606
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughReplaced many Config method receivers (s → c); added an AES‑256‑GCM encryption package (Argon2id KDF) and tests; integrated encryption key into config loading and schema; added guardrails UI pages and provider view; extended sidebar with expandable submenus; added SendJSONWithStatus; bumped multiple Go module versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as User
participant Sidebar as Sidebar Component
participant Router as Next Router
Note over Sidebar: Parent item may have subItems
Browser->>Sidebar: Click parent item
alt parent has subItems
Sidebar->>Sidebar: toggle isExpanded
Sidebar-->>Browser: render expanded subItems
else no subItems
Sidebar->>Router: navigate to item.url
Router-->>Browser: page change
end
Browser->>Sidebar: Click sub-item
Sidebar->>Router: navigate to sub-item.url
Router-->>Browser: page change
sequenceDiagram
participant App as App Startup
participant Config as Config loader
participant Encrypt as encrypt.Init
participant Store as Config store
App->>Config: LoadConfig(from file or data)
Config->>Encrypt: Init(keyFromConfig or BIFROST_ENCRYPTION_KEY)
Encrypt-->>Config: encryption initialized (or left uninitialized)
Config->>Store: persist/read config (use Encrypt.Encrypt/Decrypt when applicable)
Note right of Encrypt: Encrypt/Decrypt are no-ops when no key is set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
ui/components/sidebar.tsx (6)
382-399: Use URL instead of title for expansion state (avoid collisions/renames)Titles can change/collide. URLs are stable and unique.
Apply:
- enterpriseItems.forEach(item => { + enterpriseItems.forEach(item => { if ('subItems' in item && item.subItems && Array.isArray(item.subItems)) { const hasActiveSubItem = item.subItems.some((subItem: { url: string }) => pathname.startsWith(subItem.url)); if (hasActiveSubItem) { - newExpandedItems.add(item.title); + newExpandedItems.add(item.url); } } });
400-410: Key expansion state by URL; simplify toggleAvoid title-based state; use item.url for stability.
- const toggleItem = (title: string) => { + const toggleItem = (key: string) => { setExpandedItems(prev => { const next = new Set(prev); - if (next.has(title)) { - next.delete(title); + if (next.has(key)) { + next.delete(key); } else { - next.add(title); + next.add(key); } return next; }); };
474-499: Propagate URL-based keying to callersEnsures consistent expansion behavior.
- isExpanded={expandedItems.has(item.title)} - onToggle={() => toggleItem(item.title)} + isExpanded={expandedItems.has(item.url)} + onToggle={() => toggleItem(item.url)} - pathname={pathname} - router={router} + pathname={pathname} + router={router}Repeat the same for enterpriseItems mapping in Lines 495-499.
251-263: Add aria-expanded for accessibility on expandable itemsImprove screen-reader support.
<SidebarMenuButton className={`relative h-7.5 rounded-md border px-3 transition-all duration-200 cursor-pointer ${ /* ... */ } `} + aria-expanded={hasSubItems ? !!isExpanded : undefined} onClick={hasSubItems ? handleClick : () => handleNavigation(item.url)} >
270-274: Make parent item navigable; toggle via chevron only (better UX)Currently clicking the parent only toggles; base route (/guardrails) becomes unreachable. Consider toggling via chevron and navigating on parent click.
Example change:
- {hasSubItems && ( - <ChevronRight - className={`h-4 w-4 transition-transform duration-200 ${isExpanded ? "rotate-90" : ""}`} - /> - )} + {hasSubItems && ( + <ChevronRight + role="button" + aria-label={isExpanded ? "Collapse" : "Expand"} + onClick={(e) => { e.stopPropagation(); onToggle && onToggle(); }} + className={`h-4 w-4 transition-transform duration-200 ${isExpanded ? "rotate-90" : ""}`} + /> + )}And revert SidebarMenuButton onClick to always navigate:
onClick={() => handleNavigation(item.url)}
290-309: Use stable keys for subitemsPrefer URL as key to avoid duplicate title collisions.
- <SidebarMenuSubItem key={subItem.title}> + <SidebarMenuSubItem key={subItem.url}>ui/app/guardrails/configuration/page.tsx (1)
1-6: LGTMPage correctly renders enterprise fallback view. Consider standardizing alias usage across guardrails pages (use @enterprise consistently).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
transports/bifrost-http/lib/config.go(47 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx(1 hunks)ui/app/guardrails/configuration/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/guardrails/providers/page.tsx(1 hunks)ui/components/sidebar.tsx(8 hunks)ui/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
ui/app/guardrails/providers/page.tsx (1)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx (1)
GuardrailsProviderView(5-17)
ui/app/guardrails/configuration/page.tsx (1)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
GuardrailsConfigurationView(5-17)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (2)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsView.tsx (1)
GuardrailsView(5-17)
ui/components/sidebar.tsx (2)
ui/components/ui/sidebar.tsx (5)
SidebarMenuItem(611-611)SidebarMenuButton(610-610)SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
transports/bifrost-http/lib/config.go (5)
plugins/semanticcache/main.go (1)
Config(25-43)framework/configstore/clientconfig.go (2)
ClientConfig(28-39)EnvKeyInfo(18-24)core/schemas/account.go (4)
VertexKeyConfig(35-39)BedrockKeyConfig(45-52)Key(8-17)AzureKeyConfig(27-31)framework/configstore/store.go (1)
ConfigStore(16-118)core/schemas/mcp.go (1)
MCPConfig(11-13)
🔇 Additional comments (5)
transports/bifrost-http/lib/config.go (1)
775-2222: LGTM! Clean and consistent receiver renaming.The refactor standardizes the receiver variable from
stocacross all Config methods, preserving all logic and following Go conventions.ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx (1)
5-17: LGTM: rename to GuardrailsProviderViewDefault-export rename is safe; component content unchanged.
ui/package.json (1)
56-56: Satisfy react-querybuilder peer dependencies
react-querybuilder@8.10.0 declares peerDependencies: react >=18 (you’re on 19.1.1), json-logic-js >=2, drizzle-orm >=0.38.0, and sequelize >=6—ensure these are installed or otherwise handled. Optionally pin the spec to “8.10.0” for explicit control (your lockfile already fixes the version).ui/app/guardrails/providers/page.tsx (1)
3-4: LGTM! Straightforward page wrapper delegating to the enterprise guardrails provider view. Nicely keeps routing consistent.ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
5-16: LGTM! Matches the provider view pattern and cleanly reuses the enterprise contact surface.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
5-17: Reduce duplication between Provider and Configuration viewsBoth views render the same ContactUsView with identical props. Consider extracting a shared GuardrailsEnterpriseLanding component to keep one source of truth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
framework/encrypt/encrypt.go(1 hunks)framework/encrypt/encrypt_test.go(1 hunks)transports/bifrost-http/handlers/utils.go(1 hunks)transports/bifrost-http/lib/config.go(53 hunks)transports/config.schema.json(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx(1 hunks)ui/app/guardrails/configuration/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/guardrails/providers/page.tsx(1 hunks)ui/components/sidebar.tsx(8 hunks)ui/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/package.json
- ui/app/guardrails/page.tsx
- ui/app/guardrails/providers/page.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
transports/bifrost-http/handlers/utils.go (1)
core/schemas/logger.go (1)
Logger(28-55)
ui/app/guardrails/configuration/page.tsx (1)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
GuardrailsConfigurationView(5-17)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (2)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsView.tsx (1)
GuardrailsView(5-17)
ui/components/sidebar.tsx (2)
ui/components/ui/sidebar.tsx (5)
SidebarMenuItem(611-611)SidebarMenuButton(610-610)SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
framework/encrypt/encrypt_test.go (3)
framework/encrypt/encrypt.go (3)
Init(23-39)Encrypt(77-106)Decrypt(109-148)core/logger.go (1)
NewDefaultLogger(40-49)core/schemas/logger.go (1)
LogLevelInfo(11-11)
framework/encrypt/encrypt.go (2)
core/schemas/logger.go (1)
Logger(28-55)transports/bifrost-http/lib/config.go (1)
IsRedacted(1579-1602)
transports/bifrost-http/lib/config.go (6)
framework/encrypt/encrypt.go (1)
Init(23-39)core/schemas/bifrost.go (1)
ModelProvider(52-52)framework/configstore/clientconfig.go (3)
ProviderConfig(43-50)ClientConfig(28-39)EnvKeyInfo(18-24)core/schemas/account.go (4)
VertexKeyConfig(35-39)BedrockKeyConfig(45-52)Key(8-17)AzureKeyConfig(27-31)framework/configstore/store.go (1)
ConfigStore(16-118)core/schemas/mcp.go (1)
MCPConfig(11-13)
🔇 Additional comments (6)
transports/bifrost-http/handlers/utils.go (1)
24-32: LGTM! Well-structured helper function.The implementation correctly extends the existing
SendJSONpattern by allowing custom status codes. Error handling is consistent with the existing code.framework/encrypt/encrypt_test.go (1)
10-139: LGTM! Comprehensive test coverage.The test suite covers all critical scenarios:
- Encryption/decryption round-trip for various input types
- Non-deterministic encryption (random nonces produce different ciphertexts)
- Error handling for invalid inputs
Well-structured with table-driven tests.
transports/bifrost-http/lib/config.go (2)
804-810: LGTM! Consistent receiver rename refactor.All Config method receivers have been consistently renamed from
stocthroughout the file. The change improves code readability and follows the convention of usingcfor config-type receivers.Also applies to: 820-833, 844-856, 864-866, 904-1060, 1063-1073, 1083-1168, 1186-1267, 1270-1292, 1295-1313, 1324-1359, 1364-1369, 1378-1432, 1441-1476, 1485-1519, 1523-1556, 1612-1625, 1629-1654, 1662-1694, 1705-1749, 1751-1776, 1778-1820, 1835-1891, 1893-1934, 1937-1992, 1995-2090, 2093-2121, 2179-2218, 2220-2241
20-20: LGTM! Encryption initialization is correctly implemented.The encryption initialization is properly handled in both code paths:
- When config file doesn't exist (lines 385-390): Reads from
BIFROST_ENCRYPTION_KEYenvironment variable only- When config file exists (lines 781-799): Reads from config file with support for
env.prefix, then falls back toBIFROST_ENCRYPTION_KEYBoth paths correctly call
encrypt.Initto initialize the encryption system.Also applies to: 41-41, 58-58, 75-75, 385-390, 781-799
ui/app/guardrails/configuration/page.tsx (1)
4-6: LGTMThin page wrapper is fine.
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx (1)
5-16: LGTM: client boundary verification passedContactUsView declares
"use client", so GuardrailsProviderView can remain a server component.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ui/components/sidebar.tsx (1)
231-312: Parent items still lack navigation and accessibility improvements.The issues raised in the previous review remain unaddressed:
Navigation gap: Parent items with
subItemsonly toggle expansion (line 263 callshandleClickwhich invokesonToggle()), but never navigate toitem.url(e.g.,/guardrails). Users expect clicking the row to navigate to the parent page.Missing ARIA attributes: The expandable menu lacks screen-reader hints:
- No
aria-expandedonSidebarMenuButton(line 255)- No
aria-controlslinking the button to the submenu- No
idorrole="menu"onSidebarMenuSub(line 285)- Chevron (lines 271-274) is decorative, not an interactive button with
aria-labelChevron interaction: The chevron should be a separate focusable
<button>that stops event propagation and callsonToggle(), allowing the row click to navigate while the chevron independently toggles.Refer to the previous review for the recommended fix:
- Change line 263's
onClickto always callhandleNavigation(item.url)(respectingisAllowed)- Extract the chevron into its own
<button>withonClick={(e) => { e.stopPropagation(); onToggle?.(); }}andaria-label- Add
aria-expanded={!!isExpanded},aria-controls={subMenuId}, andaria-haspopup="menu"toSidebarMenuButton- Define
subMenuId(e.g.,submenu-${item.title.toLowerCase().replace(/\s+/g, "-")}) and setid={subMenuId}androle="menu"onSidebarMenuSubBased on previous review comments.
transports/config.schema.json (1)
8-12: Same security concern: hardcoded default encryption key persists.This was flagged in a previous review (see past review comments above) but has not been addressed. The hardcoded default value
"bifrost-default-encryption-key-32b"creates a security vulnerability:
- All deployments using the default will share the same encryption key
- Anyone with access to the schema/code can decrypt data from any deployment using defaults
- The description states "If not provided, a default key will be used" which contradicts the actual implementation in
framework/encrypt/encrypt.go(lines 25-28), where an empty key disables encryption with a warningApply the fix from the previous review comment:
Option 1 (Recommended): Remove the default to require explicit configuration:
"encryption_key": { "type": "string", - "description": "Encryption key for the configuration. If not provided, a default key will be used. You can set the value as env.<ENV_VAR_NAME> to use an environment variable. We also read encryption key from BIFROST_ENCRYPTION_KEY environment variable.", - "default": "bifrost-default-encryption-key-32b" + "description": "Encryption key for the configuration. If not provided, encryption will be disabled. You can set the value as env.<ENV_VAR_NAME> to use an environment variable, or set the BIFROST_ENCRYPTION_KEY environment variable. Note: Once an encryption key is set, it cannot be changed without re-encrypting the database." }Option 2: Set default to empty string to explicitly disable encryption:
"encryption_key": { "type": "string", - "description": "Encryption key for the configuration. If not provided, a default key will be used. You can set the value as env.<ENV_VAR_NAME> to use an environment variable. We also read encryption key from BIFROST_ENCRYPTION_KEY environment variable.", - "default": "bifrost-default-encryption-key-32b" + "description": "Encryption key for the configuration. If empty, encryption will be disabled. You can set the value as env.<ENV_VAR_NAME> to use an environment variable, or set the BIFROST_ENCRYPTION_KEY environment variable. Note: Once an encryption key is set, it cannot be changed without re-encrypting the database.", + "default": "" }Both options align with the actual behavior in
framework/encrypt/encrypt.gowhere an empty key disables encryption and logs a warning.Based on past review comments.
🧹 Nitpick comments (4)
transports/bifrost-http/handlers/utils.go (1)
24-32: LGTM! Consider refactoring to reduce duplication.The implementation is correct and follows the established pattern. The function properly sets the content type, status code, and handles encoding errors.
To reduce code duplication between
SendJSONandSendJSONWithStatus, consider refactoringSendJSONto callSendJSONWithStatuswith a default status code:// SendJSON sends a JSON response with 200 OK status func SendJSON(ctx *fasthttp.RequestCtx, data interface{}, logger schemas.Logger) { - ctx.SetContentType("application/json") - if err := json.NewEncoder(ctx).Encode(data); err != nil { - logger.Warn(fmt.Sprintf("Failed to encode JSON response: %v", err)) - SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Failed to encode response: %v", err), logger) - } + SendJSONWithStatus(ctx, data, fasthttp.StatusOK, logger) }framework/encrypt/encrypt_test.go (2)
10-75: Good test coverage for core encryption functionality.The test covers a comprehensive set of inputs (simple text, AWS keys, empty string, special characters, long text) and properly validates:
- Encryption produces different output than plaintext
- Decryption recovers the original plaintext
- Empty string handling (encryption returns empty)
Consider adding test cases for:
- Behavior when
encryptionKeyis nil (encryption disabled scenario fromencrypt.goline 25-28)- Explicit verification that the 40-byte test key is truncated to exactly 32 bytes by
Init(as perencrypt.goline 31)- Warning log verification when an empty key is passed to
Init
108-139: Good error handling coverage.Tests appropriately verify that
Decryptreturns errors for invalid inputs:
- Malformed base64
- Valid base64 with invalid ciphertext
- Ciphertext shorter than nonce size
Consider adding a test case for ciphertext that's exactly the nonce size but has no encrypted payload (edge case for line
nonceSizeinencrypt.goline 134).transports/bifrost-http/lib/config.go (1)
385-390: Consider refactoring duplicated encryption initialization logic.Encryption initialization appears in two places with similar but not identical logic. The second block (lines 781-799) is more sophisticated, supporting
env.VAR_NAMEprefix processing, while the first (lines 385-390) only checks the environment variable directly.Extract the encryption initialization logic into a helper method to eliminate duplication and ensure consistent behavior:
+// initEncryption initializes the encryption subsystem from config data and environment +func (c *Config) initEncryption(configData *ConfigData) error { + var encryptionKey string + + // First try config data if available + if configData != nil && configData.EncryptionKey != "" { + if strings.HasPrefix(configData.EncryptionKey, "env.") { + var err error + if encryptionKey, _, err = c.processEnvValue(configData.EncryptionKey); err != nil { + return fmt.Errorf("failed to process encryption key: %w", err) + } + } else { + encryptionKey = configData.EncryptionKey + } + } + + // Fall back to environment variable + if encryptionKey == "" { + if os.Getenv("BIFROST_ENCRYPTION_KEY") != "" { + encryptionKey = os.Getenv("BIFROST_ENCRYPTION_KEY") + } + } + + encrypt.Init(encryptionKey, logger) + return nil +}Then replace both initialization blocks:
- // We check the encryption key is present in the environment variables - encryptionKey := "" - if os.Getenv("BIFROST_ENCRYPTION_KEY") != "" { - encryptionKey = os.Getenv("BIFROST_ENCRYPTION_KEY") - } - encrypt.Init(encryptionKey, logger) + if err := config.initEncryption(nil); err != nil { + return nil, err + }And:
- // Initializing encryption - var encryptionKey string - if configData.EncryptionKey != "" { - if strings.HasPrefix(configData.EncryptionKey, "env.") { - if encryptionKey, _, err = config.processEnvValue(configData.EncryptionKey); err != nil { - return nil, fmt.Errorf("failed to process encryption key: %w", err) - } - } else { - encryptionKey = configData.EncryptionKey - } - } - if encryptionKey == "" { - // We will try to read from the default environment variable - if os.Getenv("BIFROST_ENCRYPTION_KEY") != "" { - encryptionKey = os.Getenv("BIFROST_ENCRYPTION_KEY") - } - } - encrypt.Init(encryptionKey, logger) - // Done initializing encryption + if err := config.initEncryption(&configData); err != nil { + return nil, err + }Also applies to: 781-799
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
framework/encrypt/encrypt.go(1 hunks)framework/encrypt/encrypt_test.go(1 hunks)transports/bifrost-http/handlers/utils.go(1 hunks)transports/bifrost-http/lib/config.go(53 hunks)transports/config.schema.json(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx(1 hunks)ui/app/guardrails/configuration/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/guardrails/providers/page.tsx(1 hunks)ui/components/sidebar.tsx(8 hunks)ui/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/app/guardrails/providers/page.tsx
- ui/app/guardrails/configuration/page.tsx
- ui/app/guardrails/page.tsx
- framework/encrypt/encrypt.go
- ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
ui/components/sidebar.tsx (2)
ui/components/ui/sidebar.tsx (5)
SidebarMenuItem(611-611)SidebarMenuButton(610-610)SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
framework/encrypt/encrypt_test.go (3)
framework/encrypt/encrypt.go (3)
Init(23-39)Encrypt(77-106)Decrypt(109-148)core/logger.go (1)
NewDefaultLogger(40-49)core/schemas/logger.go (1)
LogLevelInfo(11-11)
transports/bifrost-http/handlers/utils.go (1)
core/schemas/logger.go (1)
Logger(28-55)
transports/bifrost-http/lib/config.go (6)
framework/encrypt/encrypt.go (1)
Init(23-39)framework/configstore/clientconfig.go (3)
ProviderConfig(43-50)ClientConfig(28-39)EnvKeyInfo(18-24)core/schemas/account.go (4)
VertexKeyConfig(35-39)BedrockKeyConfig(45-52)Key(8-17)AzureKeyConfig(27-31)framework/configstore/store.go (1)
ConfigStore(16-118)framework/configstore/tables.go (4)
TableKey(59-97)TableKey(191-191)TablePlugin(176-186)TablePlugin(198-198)core/schemas/mcp.go (1)
MCPConfig(11-13)
🔇 Additional comments (7)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx (1)
5-5: LGTM! Component rename aligns with its purpose.The rename from
GuardrailsViewtoGuardrailsProviderViewaccurately reflects that this component is specifically for the providers view within the guardrails feature.ui/components/sidebar.tsx (2)
383-410: Auto-expand logic is correct and functional.The effect correctly auto-expands parent items when their sub-items are active (lines 383-398), and the
toggleItemfunction properly manages manual expansion state (lines 400-410). The implementation handles the Set-based state correctly and integrates well with the pathname-based active detection.
121-134: Sub-items structure is well-defined.The
subItemsarray for Guardrails is properly structured with Configuration and Providers entries, each having appropriate icons, URLs, and descriptions. This aligns with the broader UI expansion for enterprise guardrails features.framework/encrypt/encrypt_test.go (1)
77-106: Excellent test for GCM non-determinism.Correctly verifies that AES-GCM produces different ciphertexts for identical plaintexts due to the random nonce, while both decrypt to the same original value. This is essential behavior for cryptographic security.
transports/bifrost-http/lib/config.go (2)
20-20: Encryption integration properly added to ConfigData.The import and field additions correctly integrate the encryption package:
- Import added for
framework/encryptEncryptionKeyfield added toConfigDataandTempConfigData- Field properly set during unmarshalling
Also applies to: 41-41, 58-58, 75-75
804-804: Receiver naming standardization applied consistently.The receiver variable has been correctly renamed from
stocthroughout the file for allConfigmethods. This improves consistency with the struct name (Config) and makes the code more readable.Sample methods verified:
GetRawConfigString(line 804)processEnvValue(line 820)GetProviderConfigRaw(lines 844-846)ShouldAllowDirectKeys(lines 864-865)GetProviderConfigRedacted(lines 904-906)GetAllProviders(lines 1063-1065)AddProvider(lines 1083-1085)UpdateProviderConfig(lines 1186-1188)RemoveProvider(lines 1269-1272)GetAllKeys(lines 1295-1297)All receiver references (
c.Mu,c.Providers,c.EnvKeys,c.ConfigStore, etc.) are correctly updated.Also applies to: 820-820, 844-846, 864-865, 904-906, 1063-1065, 1083-1085, 1186-1188, 1269-1272, 1295-1297
ui/package.json (1)
56-56: react-querybuilder@8.10.0 supports React 19
Peer dependency specifies"react": ">=18", so React 19 is satisfied.
akshaydeo
force-pushed
the
guardrails
branch
2 times, most recently
from
9c591dc to
0931784
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
ui/app/guardrails/page.tsx (1)
1-1: Fix critical import name mismatch and path.The import has multiple critical issues:
- Name mismatch: The component is exported as
GuardrailsProviderViewbut imported asGuardrailsView, causing a build failure.- Path mismatch: The file is located at
ui/app/_fallbacks/enterprise/...but the import references@/app/enterprise/....- Alias usage: As flagged in the previous review, use
@enterprisealias for consistency.Apply this diff to fix all issues:
-import GuardrailsView from "@/app/enterprise/components/guardrails/guardrailsProviderView"; +import GuardrailsProviderView from "@enterprise/components/guardrails/guardrailsProviderView";And update the component usage:
export default function GuardrailsPage() { - return <GuardrailsView />; + return <GuardrailsProviderView />; }ui/components/sidebar.tsx (1)
256-298: Restore navigation for parent menu itemsWith the new toggle-only handler, clicking the Guardrails parent never routes to
/guardrails, so that page is now unreachable. We still need the row click (or an equivalent primary action) to navigate while a separate control handles expand/collapse—with the appropriate ARIA wiring noted previously. This is the same issue flagged in the earlier review and remains unresolved.framework/encrypt/encrypt.go (1)
77-107: Silent failures in Encrypt could lead to unencrypted data being stored.This issue was already flagged in a previous review. The
Encryptfunction returns plaintext on any encryption error (lines 88, 93, 99) without logging the failure, which could result in sensitive data being stored unencrypted without detection.As suggested in the previous review, consider either:
- Logging all encryption failures before returning plaintext, or
- Changing the function signature to return
(string, error)and letting callers handle failures
🧹 Nitpick comments (2)
framework/encrypt/encrypt.go (1)
61-75: Inconsistent redaction format compared to RedactKey in config.go.This
RedactSecretfunction produces a different redaction format thanRedactKeyin config.go (lines 1559-1575):
RedactSecret: Shows last 4 chars with variable-length asterisks prefix (e.g.,"****abc"for 7-char secret)RedactKey: Shows first 4 and last 4 with exactly 24 asterisks in middle (always 32 chars, e.g.,"sk-1************************xyz7")This inconsistency could confuse users and make it harder to match redacted values with their original format. Consider standardizing on one format.
Additionally,
IsRedacted(lines 42-59) checks for theRedactSecretpattern but not theRedactKeypattern, which could lead to false negatives.transports/bifrost-http/lib/config.go (1)
385-390: Validate encryption key length before initialization.The code retrieves the encryption key from the environment but doesn't validate its length before passing it to
encrypt.Init. Given thatInithas weak key padding logic (pads short keys with zeros), it's better to validate the key length here and fail fast with a clear error message.Apply this diff to add validation:
// We check the encryption key is present in the environment variables encryptionKey := "" if os.Getenv("BIFROST_ENCRYPTION_KEY") != "" { encryptionKey = os.Getenv("BIFROST_ENCRYPTION_KEY") } + if encryptionKey != "" && len(encryptionKey) != 32 { + return nil, fmt.Errorf("encryption key must be exactly 32 bytes for AES-256, got %d bytes", len(encryptionKey)) + } encrypt.Init(encryptionKey, logger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
core/go.mod(2 hunks)framework/encrypt/encrypt.go(1 hunks)framework/encrypt/encrypt_test.go(1 hunks)framework/go.mod(2 hunks)plugins/governance/go.mod(1 hunks)plugins/jsonparser/go.mod(1 hunks)plugins/logging/go.mod(1 hunks)plugins/maxim/go.mod(1 hunks)plugins/mocker/go.mod(1 hunks)plugins/otel/go.mod(1 hunks)plugins/semanticcache/go.mod(1 hunks)plugins/telemetry/go.mod(1 hunks)transports/bifrost-http/handlers/utils.go(1 hunks)transports/bifrost-http/lib/config.go(53 hunks)transports/config.schema.json(1 hunks)transports/go.mod(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx(1 hunks)ui/app/guardrails/configuration/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/guardrails/providers/page.tsx(1 hunks)ui/components/sidebar.tsx(8 hunks)ui/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- transports/bifrost-http/handlers/utils.go
- ui/app/guardrails/providers/page.tsx
- transports/config.schema.json
- ui/app/guardrails/configuration/page.tsx
- ui/package.json
- framework/encrypt/encrypt_test.go
- core/go.mod
- plugins/otel/go.mod
- plugins/governance/go.mod
- plugins/mocker/go.mod
- plugins/telemetry/go.mod
- framework/go.mod
- plugins/logging/go.mod
- plugins/jsonparser/go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
framework/encrypt/encrypt.go (2)
core/schemas/logger.go (1)
Logger(28-55)transports/bifrost-http/lib/config.go (1)
IsRedacted(1579-1602)
ui/components/sidebar.tsx (1)
ui/components/ui/sidebar.tsx (5)
SidebarMenuItem(611-611)SidebarMenuButton(610-610)SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)
transports/bifrost-http/lib/config.go (6)
framework/encrypt/encrypt.go (1)
Init(23-40)framework/configstore/clientconfig.go (3)
ProviderConfig(43-50)ClientConfig(28-39)EnvKeyInfo(18-24)core/schemas/account.go (4)
VertexKeyConfig(35-39)BedrockKeyConfig(45-52)Key(8-17)AzureKeyConfig(27-31)framework/configstore/store.go (1)
ConfigStore(16-118)framework/configstore/tables.go (4)
TableKey(59-97)TableKey(191-191)TablePlugin(176-186)TablePlugin(198-198)core/schemas/mcp.go (1)
MCPConfig(11-13)
🪛 Gitleaks (8.28.0)
framework/encrypt/encrypt.go
[high] 20-20: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
transports/bifrost-http/lib/config.go (1)
41-41: LGTM!The
EncryptionKeyfield is properly added toConfigDataand correctly handled in theUnmarshalJSONmethod.Also applies to: 58-58, 75-75
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
ui/app/guardrails/page.tsx (1)
1-1: Use@enterprisealias for consistency.The import path was previously flagged for using a long absolute path instead of the project's
@enterprisealias. This inconsistency should be addressed for maintainability.Apply this diff:
-import GuardrailsView from "@/app/enterprise/components/guardrails/guardrailsProviderView"; +import GuardrailsView from "@enterprise/components/guardrails/guardrailsProviderView";ui/components/sidebar.tsx (1)
263-312: Parent menu remains non-navigable and inaccessibleClicking a parent with subItems still only toggles the submenu—it never routes to
item.url, so/guardrailsis unreachable via the sidebar. The chevron also isn’t a focusable, labeled control, so screen-reader and keyboard users can’t understand or toggle the submenu. Please restore navigation on the parent row, move the toggle into its own<button>witharia-label/aria-expanded, and wirearia-controls/idbetween the row and<SidebarMenuSub>(e.g., via a deterministicsubMenuId).- const handleClick = (e: React.MouseEvent) => { - if (hasSubItems && onToggle) { - e.preventDefault(); - onToggle(); - } - }; + const subMenuId = hasSubItems ? `submenu-${item.title.toLowerCase().replace(/\s+/g, "-")}` : undefined; + + const handleToggleClick = (e: React.MouseEvent) => { + if (!hasSubItems || !onToggle) return; + e.stopPropagation(); + onToggle(); + }; @@ - <SidebarMenuButton + <SidebarMenuButton className={`relative h-7.5 rounded-md border px-3 transition-all duration-200 cursor-pointer ${ @@ - } `} - onClick={hasSubItems ? handleClick : () => handleNavigation(item.url)} + } `} + onClick={() => handleNavigation(item.url)} + aria-haspopup={hasSubItems ? "menu" : undefined} + aria-expanded={hasSubItems ? !!isExpanded : undefined} + aria-controls={subMenuId} > @@ - {hasSubItems && ( - <ChevronRight - className={`h-4 w-4 transition-transform duration-200 ${isExpanded ? "rotate-90" : ""}`} - /> - )} + {hasSubItems && ( + <button + type="button" + aria-label={isExpanded ? `Collapse ${item.title}` : `Expand ${item.title}`} + aria-expanded={!!isExpanded} + onClick={handleToggleClick} + className="ml-2 rounded p-1 hover:bg-sidebar-accent focus:outline-none focus-visible:ring-2 focus-visible:ring-primary" + > + <ChevronRight + className={`h-4 w-4 transition-transform duration-200 ${isExpanded ? "rotate-90" : ""}`} + /> + </button> + )} @@ -{hasSubItems && isExpanded && ( - <SidebarMenuSub className="ml-4 border-l border-sidebar-border pl-2 space-y-0.5 mt-1"> +{hasSubItems && isExpanded && ( + <SidebarMenuSub + id={subMenuId} + className="ml-4 border-l border-sidebar-border pl-2 space-y-0.5 mt-1" + role="menu" + >
🧹 Nitpick comments (1)
framework/encrypt/encrypt.go (1)
18-44: Consider protecting against concurrent Init calls.The package-level
encryptionKeyandloggervariables are mutable and accessed without synchronization. IfInitis called concurrently (e.g., during tests or reinitialization), race conditions could occur.Consider using
sync.Onceto ensureInitis only called once, or add synchronization if reinitialization is intended:var ( encryptionKey []byte logger schemas.Logger initOnce sync.Once ) func Init(key string, _logger schemas.Logger) { initOnce.Do(func() { logger = _logger // ... rest of initialization }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
core/go.mod(2 hunks)framework/encrypt/encrypt.go(1 hunks)framework/encrypt/encrypt_test.go(1 hunks)framework/go.mod(2 hunks)plugins/governance/go.mod(1 hunks)plugins/jsonparser/go.mod(1 hunks)plugins/logging/go.mod(1 hunks)plugins/maxim/go.mod(1 hunks)plugins/mocker/go.mod(1 hunks)plugins/otel/go.mod(1 hunks)plugins/semanticcache/go.mod(1 hunks)plugins/telemetry/go.mod(1 hunks)transports/bifrost-http/handlers/utils.go(1 hunks)transports/bifrost-http/lib/config.go(53 hunks)transports/config.schema.json(1 hunks)transports/go.mod(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx(1 hunks)ui/app/guardrails/configuration/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/guardrails/providers/page.tsx(1 hunks)ui/components/sidebar.tsx(8 hunks)ui/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- ui/app/guardrails/configuration/page.tsx
- ui/app/guardrails/providers/page.tsx
- plugins/telemetry/go.mod
- core/go.mod
- plugins/mocker/go.mod
- ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx
- plugins/logging/go.mod
- plugins/otel/go.mod
- plugins/jsonparser/go.mod
- transports/config.schema.json
- transports/go.mod
🧰 Additional context used
🧬 Code graph analysis (5)
transports/bifrost-http/handlers/utils.go (1)
core/schemas/logger.go (1)
Logger(28-55)
framework/encrypt/encrypt_test.go (3)
framework/encrypt/encrypt.go (3)
Init(24-44)Encrypt(47-76)Decrypt(79-118)core/logger.go (1)
NewDefaultLogger(40-49)core/schemas/logger.go (1)
LogLevelInfo(11-11)
framework/encrypt/encrypt.go (1)
core/schemas/logger.go (1)
Logger(28-55)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
transports/bifrost-http/lib/config.go (3)
framework/encrypt/encrypt.go (1)
Init(24-44)framework/configstore/clientconfig.go (3)
ProviderConfig(43-50)ClientConfig(28-39)EnvKeyInfo(18-24)framework/configstore/store.go (1)
ConfigStore(16-118)
🔇 Additional comments (14)
ui/package.json (1)
56-56: No action needed: react-querybuilder supports React 19. Peer dependency"react": ">=18"covers React 19.1.1.ui/app/guardrails/page.tsx (1)
5-7: LGTM!The component implementation is clean and serves its purpose as a simple page wrapper.
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (2)
1-2: LGTM!The imports are correct and follow best practices.
Constructionis available in lucide-react@0.542.0, and the relative import path forContactUsViewaligns with the provided code snippets.
5-17: Approve component; docs URL is reachable
HTTP 200 confirms the documentation link is accessible. Component is clean and follows theContactUsViewpattern.Optional improvements:
- Define an empty
GuardrailsConfigurationViewPropsinterface.- Extract title/description strings for future i18n.
transports/bifrost-http/handlers/utils.go (1)
24-32: LGTM!The new
SendJSONWithStatusfunction correctly extendsSendJSONwith custom status code support, following the existing error handling pattern.framework/encrypt/encrypt.go (3)
33-43: LGTM! KDF implementation addresses prior concerns.The Argon2id-based key derivation properly handles arbitrary passphrase lengths and provides strong entropy. The fixed salt is appropriate for system-wide encryption (not per-user credentials). The warning for short passphrases helps guide users toward better security.
Note: This addresses the "weak key normalization" concern from previous reviews by using a proper KDF instead of padding with zeros.
46-76: LGTM! Error handling improvements applied.The function now returns errors instead of silently falling back to plaintext, addressing the concern from previous reviews about silent failures leading to unencrypted data storage.
Note: Callers can now detect encryption failures and handle them appropriately.
78-118: LGTM! Decrypt implementation is correct.The decryption logic properly validates input, extracts the nonce, and handles errors. The early return for uninitialized keys and empty input is appropriate.
framework/encrypt/encrypt_test.go (3)
80-115: LGTM! Nonce randomness verification is correct.The test properly verifies that two encryptions of the same plaintext produce different ciphertexts (due to random nonces) but both decrypt to the original plaintext. This confirms proper nonce usage.
150-203: LGTM! KDF key length tests are comprehensive.The tests verify that keys of various lengths (8, 16, 32, 64 bytes) all work correctly with the KDF, confirming that Argon2id properly derives a secure 32-byte key from any input.
205-245: LGTM! KDF determinism test is essential.This test confirms that re-initializing with the same passphrase produces the same derived key, which is critical for being able to decrypt previously encrypted data after a restart.
transports/bifrost-http/lib/config.go (3)
41-41: LGTM! EncryptionKey field added to config structures.The
EncryptionKeyfield is properly integrated intoConfigDataandTempConfigData, with correct JSON unmarshaling.Also applies to: 58-58, 75-75
808-814: LGTM! Receiver naming standardized fromstoc.The consistent use of
cas the receiver name across allConfigmethods improves code readability and maintainability.Also applies to: 824-837, 848-860, 868-870, 908-914, 1067-1077, 1087-1172, 1190-1271, 1274-1296, 1299-1317, 1328-1363, 1368-1436, 1445-1480, 1489-1523, 1527-1560, 1616-1658, 1666-1698, 1709-1753, 1755-1780, 1783-1824, 1839-1895, 1898-1938, 1941-1996, 1999-2094, 2097-2125, 2183-2222, 2224-2245
781-790: LGTM! Security warning for config file storage.The warning on line 789 appropriately advises users to reference environment variables instead of storing keys directly in config files, addressing the security concern from previous reviews.
Based on learnings: This aligns with best practices for secret management in configuration.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx (1)
5-5: Previously flagged: Verify import consistency.This function rename was already flagged in a previous review. The concern about ui/app/guardrails/page.tsx still importing the old name
GuardrailsViewremains valid based on the AI summary.ui/components/sidebar.tsx (1)
231-314: Previously flagged: Parent menu navigation and accessibility.The concerns raised in the previous review about parent item navigation and ARIA attributes remain valid:
- Parent items with subItems (e.g., /guardrails) cannot be navigated to—they only toggle the submenu
- Missing ARIA attributes for screen reader accessibility
- Chevron should be a separate focusable button to allow users to navigate to parent vs. toggle submenu
framework/encrypt/encrypt.go (2)
1-3: Update stale package comment.This is now in active use; drop the “We are not using it anywhere yet” sentence.
Apply:
-// We are not using it anywhere yet - we will introduce encryption for all the sensitive data in one go to avoid breaking changes
24-31: Don’t silently disable encryption when key is empty.Init returns with no log; this can lead to plaintext being stored without any visibility.
Apply:
func Init(key string, _logger schemas.Logger) { logger = _logger if key == "" { - // TODO uncomment this warning when we have full coverage for encryption - // In this case encryption will be disabled - // logger.Warn("encryption key is not set, encryption will be disabled. To set encryption key: use the encryption_key field in the configuration file or set the BIFROST_ENCRYPTION_KEY environment variable. Note that - once encryption key is set, it cannot be changed later unless you clean up the database.") - return + logger.Warn("encryption key is not set; encryption is DISABLED. Set BIFROST_ENCRYPTION_KEY or use encryption_key with env.VAR_NAME.") + return }
🧹 Nitpick comments (2)
ui/components/sidebar.tsx (1)
382-398: Auto-expansion state persists across navigation.The auto-expansion effect only adds items to
expandedItemsbut never removes them. Once a parent item is auto-expanded (because a subItem is active), it remains expanded even after navigating away.If you want the submenu to collapse when navigating away from its children, replace the expansion logic:
-useEffect(() => { - const newExpandedItems = new Set<string>(); - - enterpriseItems.forEach(item => { - if ('subItems' in item && item.subItems && Array.isArray(item.subItems)) { - const hasActiveSubItem = item.subItems.some((subItem: { url: string }) => pathname.startsWith(subItem.url)); - if (hasActiveSubItem) { - newExpandedItems.add(item.title); - } - } - }); - - if (newExpandedItems.size > 0) { - setExpandedItems(prev => new Set([...prev, ...newExpandedItems])); - } -}, [pathname]); +useEffect(() => { + const newExpandedItems = new Set<string>(); + + enterpriseItems.forEach(item => { + if ('subItems' in item && item.subItems && Array.isArray(item.subItems)) { + const hasActiveSubItem = item.subItems.some(subItem => pathname.startsWith(subItem.url)); + if (hasActiveSubItem) { + newExpandedItems.add(item.title); + } + } + }); + + setExpandedItems(newExpandedItems); +}, [pathname]);This will ensure that only parents with currently active children remain expanded.
Note: The current behavior (persist expansion) may be intentional for better UX—keeping recently viewed sections open.
transports/bifrost-http/lib/config.go (1)
41-41: Avoid persisting/serving encryption_key in config payloads.Including this field risks accidental leakage via “raw config” endpoints or logs. Prefer env references only and omit from any responses.
- Keep it input-only (read on load), but do not write it back when serializing.
- Consider validating that, if present, it’s env.* (see previous comment).
Also applies to: 58-58, 75-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sumui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
core/go.mod(2 hunks)framework/encrypt/encrypt.go(1 hunks)framework/encrypt/encrypt_test.go(1 hunks)framework/go.mod(2 hunks)plugins/governance/go.mod(1 hunks)plugins/jsonparser/go.mod(1 hunks)plugins/logging/go.mod(1 hunks)plugins/maxim/go.mod(1 hunks)plugins/mocker/go.mod(1 hunks)plugins/otel/go.mod(1 hunks)plugins/semanticcache/go.mod(1 hunks)plugins/telemetry/go.mod(1 hunks)transports/bifrost-http/handlers/utils.go(1 hunks)transports/bifrost-http/lib/config.go(53 hunks)transports/config.schema.json(1 hunks)transports/go.mod(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx(1 hunks)ui/app/_fallbacks/enterprise/components/guardrails/guardrailsProviderView.tsx(1 hunks)ui/app/guardrails/configuration/page.tsx(1 hunks)ui/app/guardrails/page.tsx(1 hunks)ui/app/guardrails/providers/page.tsx(1 hunks)ui/components/sidebar.tsx(8 hunks)ui/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- ui/app/guardrails/providers/page.tsx
- ui/package.json
- ui/app/guardrails/configuration/page.tsx
- ui/app/guardrails/page.tsx
- framework/encrypt/encrypt_test.go
- framework/go.mod
- transports/config.schema.json
- transports/bifrost-http/handlers/utils.go
- plugins/logging/go.mod
- plugins/telemetry/go.mod
- transports/go.mod
- plugins/otel/go.mod
- core/go.mod
🧰 Additional context used
🧬 Code graph analysis (4)
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx (1)
ui/app/_fallbacks/enterprise/components/views/contactUsView.tsx (1)
ContactUsView(15-46)
ui/components/sidebar.tsx (2)
ui/components/ui/sidebar.tsx (5)
SidebarMenuItem(611-611)SidebarMenuButton(610-610)SidebarMenuSub(613-613)SidebarMenuSubItem(615-615)SidebarMenuSubButton(614-614)ui/components/ui/tooltip.tsx (4)
TooltipProvider(43-43)Tooltip(43-43)TooltipTrigger(43-43)TooltipContent(43-43)
transports/bifrost-http/lib/config.go (5)
framework/configstore/clientconfig.go (3)
ProviderConfig(43-50)ClientConfig(28-39)EnvKeyInfo(18-24)core/schemas/account.go (4)
VertexKeyConfig(35-39)BedrockKeyConfig(45-52)Key(8-17)AzureKeyConfig(27-31)framework/configstore/store.go (1)
ConfigStore(16-118)framework/configstore/tables.go (4)
TableKey(59-97)TableKey(191-191)TablePlugin(176-186)TablePlugin(198-198)core/schemas/mcp.go (1)
MCPConfig(11-13)
framework/encrypt/encrypt.go (1)
core/schemas/logger.go (1)
Logger(28-55)
🔇 Additional comments (7)
plugins/mocker/go.mod (1)
15-28: AWS SDK patch bumps look good.Version bumps stay within patch range and align with the broader repo updates, so there’s no expected API impact.
plugins/jsonparser/go.mod (1)
12-25: Consistent dependency upgrades.Matching AWS SDK patch upgrades here keeps the plugin aligned with the rest of the repo without altering public APIs.
ui/components/sidebar.tsx (4)
6-6: LGTM: Imports are correct.All new imports (Boxes, Cog, submenu components, ChevronRight, useRouter) are properly used throughout the component.
Also applies to: 10-10, 31-33, 40-40, 44-44
121-134: LGTM: Guardrails submenu structure is correct.The subItems structure aligns with the new guardrails pages and uses appropriate icons for each section.
212-230: LGTM: SidebarItem props extension is correct.The new props (isExpanded, onToggle, pathname, router) properly support the submenu functionality and are correctly typed.
474-477: LGTM: Props are correctly passed to SidebarItem.The expanded state, toggle handler, pathname, and router are properly passed to both standard and enterprise sidebar items.
Also applies to: 495-498
transports/bifrost-http/lib/config.go (1)
385-392: Ensure disabled encryption is logged
Verify thatinitializeEncryptionemits a clear log or warning whenencryptionKeyis empty (encryption disabled), so downstream encrypt calls don’t silently return plaintext.
ui/app/_fallbacks/enterprise/components/guardrails/guardrailsConfigurationView.tsx
Show resolved
Hide resolved
Merge activity
|
1 participant
Summary
Refactored the Config struct methods in the bifrost-http transport to use consistent receiver variable naming, replacing
swithcthroughout the codebase for better readability and consistency. Also removed unused code related to MCP configuration restoration.Changes
stocacross all Config methodsgetRestoredMCPConfigfunction that was no longer neededType of change
Affected areas
How to test
Verify that all functionality continues to work as expected:
Breaking changes
Related issues
Improves code quality and maintainability.
Security considerations
No security implications as this is a refactoring change only.
Checklist
docs/contributing/README.mdand followed the guidelines