fix(semantic-cache): harden direct-only config handling and align UI types#2358
Merged
fix(semantic-cache): harden direct-only config handling and align UI types#2358
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a required numeric Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "Web UI (pluginsForm)"
participant API as "Bifrost HTTP (config.go)"
participant Plugin as "SemanticCache (main.go)"
participant Prov as "Provider (optional)"
User->>UI: Edit/save cache config (dimension, provider, embedding_model)
UI->>API: Send normalized config (omit provider for direct-only)
API->>API: semanticCacheConfigDimension validation
alt dimension == 1 and provider omitted
API->>Plugin: Persist direct-only config (no provider keys)
Plugin->>Plugin: Init logs direct-only startup
else provider present and dimension >= 2
API->>Prov: Resolve provider keys
Prov-->>API: Return keys
API->>Plugin: Persist provider-backed config (with keys)
Plugin->>Plugin: Init uses provider-backed semantic cache
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Confidence Score: 5/5Safe to merge — all changes are validation/logging/type hardening with comprehensive test coverage and no new runtime logic paths. All findings are P2 or lower. The JSON schema if/then/else logic is correct for all four dimension/provider combinations. The Go numeric type handling is thorough. The UI discriminated union correctly separates edit-state from save-state. Previous PR thread concerns are addressed. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix: drop stale provider-backed fields i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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)
transports/config.schema.json (1)
1132-1243:⚠️ Potential issue | 🟠 MajorTighten the provider-backed branch in the semantic-cache schema.
This
allOfkeys offdimension === 1, butdimension: 1plus a provider is still provider-backed. As written, configs like{"dimension":1,"provider":"openai"}pass withoutembedding_model, andprovidercan even be an empty string. The authoritative schema should instead enforce: no provider => direct-only (dimension: 1); provider present => non-empty provider +embedding_model.Suggested fix
"provider": { "type": "string", + "minLength": 1, "description": "Provider to use for generating embeddings. Required for semantic search; omit it for direct hash mode with dimension: 1.", "enum": [ "openai", @@ "allOf": [ { "if": { - "properties": { - "dimension": { - "const": 1 - } - }, "required": [ - "dimension" + "provider" ] }, - "then": {}, - "else": { + "then": { "required": [ - "provider" + "provider", + "embedding_model" ] + }, + "else": { + "properties": { + "dimension": { + "const": 1 + } + } } } ],As per coding guidelines,
transports/config.schema.jsonis the authoritative definition for allconfig.jsonfields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 1132 - 1243, The schema's allOf currently only checks dimension===1 so {"dimension":1,"provider":"openai"} bypasses embedding_model and allows empty provider; update the conditional logic to instead branch on presence of "provider": add an if that tests for a non-empty string provider (e.g., "properties":{"provider":{"type":"string","minLength":1}},"required":["provider"]) and in that then require "embedding_model" and enforce "dimension" > 1 (or minimum 2), and in the else require "dimension" to be exactly 1 (const:1); also ensure the "provider" property itself has type:string and minLength:1 so empty strings are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 3455-3460: After trimming the provider string, write the
normalized value back into the configuration map so later consumers don't see
whitespace-padded values: after provider = strings.TrimSpace(provider) set
configMap["provider"] = provider (use the same configMap and "provider" key
referenced earlier where providerVal and provider are used).
- Around line 3441-3453: semanticCacheConfigDimension can return
hasDimension=false for missing dimension, which lets configs with a provider but
no dimension slip through; update the LoadConfig logic that handles configMap so
when a provider is present (configMap["provider"] exists) you enforce that
hasDimension is true and dimension > 1 and return a clear error otherwise (refer
to semanticCacheConfigDimension, LoadConfig, configMap, provider, dimension,
hasDimension).
In `@ui/lib/types/config.ts`:
- Around line 521-525: The CacheConfig interface currently relaxes the backend
contract by making embedding_model and dimension optional; restore the strict
canonical shape used by the backend by making embedding_model required (except
where explicit direct-only mode is modeled) and making dimension required
(number) so configs the server expects cannot be constructed with missing
fields. Update the CacheConfig declaration (referencing CacheConfig,
embedding_model, dimension, ModelProviderName, ModelProviderKey) to match the
Go-backed shape and, if you need an editor-only partial state, introduce a
separate EditorCacheConfig or a discriminated union for incomplete forms instead
of changing the canonical CacheConfig.
---
Outside diff comments:
In `@transports/config.schema.json`:
- Around line 1132-1243: The schema's allOf currently only checks dimension===1
so {"dimension":1,"provider":"openai"} bypasses embedding_model and allows empty
provider; update the conditional logic to instead branch on presence of
"provider": add an if that tests for a non-empty string provider (e.g.,
"properties":{"provider":{"type":"string","minLength":1}},"required":["provider"])
and in that then require "embedding_model" and enforce "dimension" > 1 (or
minimum 2), and in the else require "dimension" to be exactly 1 (const:1); also
ensure the "provider" property itself has type:string and minLength:1 so empty
strings are rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8021f30a-329b-4de1-934a-dd911c510a55
📒 Files selected for processing (9)
docs/features/semantic-caching.mdxplugins/semanticcache/main.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/semantic_cache_config_test.gotransports/bifrost-http/lib/validator_test.gotransports/config.schema.jsonui/app/workspace/config/views/pluginsForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 3448-3452: The early-return branches that allow direct-only mode
(where hasDimension && dimension == 1) currently return without removing any
existing embedding_model from configMap; update the validation logic in the
function handling the semantic_cache config so that before returning for
direct-only (dimension == 1) you explicitly delete or clear the
"embedding_model" entry from configMap (reference the configMap variable and the
"embedding_model" key), and apply the same removal in the other symmetric
early-return branch (the one around lines 3463-3465) so toggling from
provider-backed to direct-only no longer leaves stale embedding_model data.
In `@transports/config.schema.json`:
- Around line 1225-1258: The schema's conditional allOf currently makes
embedding_model required when provider is present but still allows
embedding_model to appear in the else branch, and the field metadata still says
it's optional; update the allOf conditional so the else branch explicitly
forbids embedding_model (e.g., add a rule that embedding_model must not be
present when provider is absent) and ensure the embedding_model property
metadata/documentation reflects that embedding_model is direct-only (required
when provider exists, disallowed otherwise); locate the allOf block containing
provider, embedding_model and dimension and adjust the else sub-schema and
embedding_model metadata accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4526334-a398-463d-b192-5e8c04d3b9ac
📒 Files selected for processing (5)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/semantic_cache_config_test.gotransports/bifrost-http/lib/validator_test.gotransports/config.schema.jsonui/app/workspace/config/views/pluginsForm.tsx
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/lib/semantic_cache_config_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- transports/bifrost-http/lib/validator_test.go
- ui/app/workspace/config/views/pluginsForm.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/app/workspace/config/views/pluginsForm.tsx (1)
351-370: Consider adding helper text clarifying dimension semantics.The dimension input now allows
min="1", but users may not understand that1means direct-only mode (hash-based, no embeddings) vs>= 2for provider-backed semantic matching. Adding a brief helper text similar to other fields would improve discoverability.💡 Suggested enhancement
<div className="space-y-2"> <Label htmlFor="dimension">Dimension</Label> <Input id="dimension" type="number" min="1" value={cacheConfig.dimension === undefined || Number.isNaN(cacheConfig.dimension) ? '' : cacheConfig.dimension} onChange={(e) => { const value = e.target.value if (value === '') { updateCacheConfigLocal({ dimension: undefined }) return } const parsed = parseInt(value) if (!Number.isNaN(parsed)) { updateCacheConfigLocal({ dimension: parsed }) } }} /> + <p className="text-muted-foreground text-xs"> + Use 1 for direct (hash-based) caching without embeddings, or the embedding model's dimension (e.g., 1536) for semantic similarity matching. + </p> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/config/views/pluginsForm.tsx` around lines 351 - 370, Add a short helper text under the Dimension field explaining semantics: that dimension = 1 means direct-only (hash-based, no embeddings) while values >= 2 enable provider-backed semantic matching; locate the Dimension input (Label with htmlFor="dimension" and Input id="dimension") and render the helper text similarly to other fields in this file (use the same styling pattern as other helper notes), referencing cacheConfig.dimension and updateCacheConfigLocal where appropriate so the text appears directly beneath the Input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Around line 351-370: Add a short helper text under the Dimension field
explaining semantics: that dimension = 1 means direct-only (hash-based, no
embeddings) while values >= 2 enable provider-backed semantic matching; locate
the Dimension input (Label with htmlFor="dimension" and Input id="dimension")
and render the helper text similarly to other fields in this file (use the same
styling pattern as other helper notes), referencing cacheConfig.dimension and
updateCacheConfigLocal where appropriate so the text appears directly beneath
the Input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d83f0da-5e87-4fe7-80e5-9cb113d8b6a6
📒 Files selected for processing (5)
transports/bifrost-http/lib/validator_test.gotransports/config.schema.jsonui/app/workspace/config/views/pluginsForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/bifrost-http/lib/validator_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 3449-3451: When normalizing direct-only mode in the configMap (the
branch that checks hasDimension && dimension == 1 and the similar branch later),
also remove any provider-backed fields so stale credentials are not kept:
specifically delete configMap["keys"] (and ensure configMap["provider"] is
removed if present) before returning; update both places (the block around the
check for hasDimension && dimension == 1 and the analogous block at 3464-3467)
to drop "keys" (and "provider") along with "embedding_model".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f416e56-a720-4ccf-a3a0-3e1860322c15
📒 Files selected for processing (2)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/semantic_cache_config_test.go
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/lib/semantic_cache_config_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/lib/semantic_cache_config_test.go (1)
53-174: Comprehensive error case coverage.The tests thoroughly validate:
- Provider key injection in provider-backed mode
- All required-field error conditions
- Invalid dimension values (0, negative, dimension=1 with provider)
Consider adding tests for edge cases like:
json.Numberorfloat64dimension values (to exercise type conversion paths insemanticCacheConfigDimension)- Whitespace-padded
provider/embedding_modelvalues (to verify trimming)- Empty string
providerwithdimension: 1(to cover the second cleanup branch at lines 3464-3470 in config.go)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/semantic_cache_config_test.go` around lines 53 - 174, Add unit tests covering edge cases called out: create tests that pass dimension as json.Number and as float64 to exercise semanticCacheConfigDimension conversion paths, tests that set provider and embedding_model with surrounding whitespace to verify trimming behavior in AddProviderKeysToSemanticCacheConfig, and a test with provider as an empty string and dimension: 1 to trigger the cleanup branch in AddProviderKeysToSemanticCacheConfig (the branch that removes provider when it's empty). Use the same test pattern as existing cases (construct PluginConfig with Config map[string]interface{}, call AddProviderKeysToSemanticCacheConfig, and assert error or injected keys) and reference the functions semanticCacheConfigDimension and AddProviderKeysToSemanticCacheConfig when locating the logic to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@transports/bifrost-http/lib/semantic_cache_config_test.go`:
- Around line 53-174: Add unit tests covering edge cases called out: create
tests that pass dimension as json.Number and as float64 to exercise
semanticCacheConfigDimension conversion paths, tests that set provider and
embedding_model with surrounding whitespace to verify trimming behavior in
AddProviderKeysToSemanticCacheConfig, and a test with provider as an empty
string and dimension: 1 to trigger the cleanup branch in
AddProviderKeysToSemanticCacheConfig (the branch that removes provider when it's
empty). Use the same test pattern as existing cases (construct PluginConfig with
Config map[string]interface{}, call AddProviderKeysToSemanticCacheConfig, and
assert error or injected keys) and reference the functions
semanticCacheConfigDimension and AddProviderKeysToSemanticCacheConfig when
locating the logic to exercise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3cf02e8e-4bc6-438b-b7ec-68fb7782dd0d
📒 Files selected for processing (2)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/semantic_cache_config_test.go
akshaydeo
approved these changes
Mar 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a contract mismatch in semantic cache direct-only mode. The docs and runtime plugin already supported the documented
dimension: 1/ no-provider setup, but the schema, transport config loading, and UI contract had not been updated to match. As a result, a valid direct-only config was either rejected, warned on, or represented inconsistently across setup paths.This PR makes that setup work cleanly through schema validation, config loading, plugin initialization, docs, and the Web UI contract. It aligns the product around two supported modes:
provider+embedding_model+dimension > 1dimension: 1with no providerChanges
dimensionrequired and only requireproviderfor semantic modekeysin config-driven setupdimension: 1with no provider as valid direct-only mode< 1) and improved error messages for semantic-mode misconfigurationinfoinstead ofwarningdimension, explain direct-only setup, and clarify thatdimension: 1plus provider is still provider-backed modeType of Change
Affected Areas
How to Test
Run targeted Go tests:
Run UI typecheck:
cd ui bun x tsc --noEmitEnd-to-end docs-style direct-only verification with Redis:
config.jsonusingdimension: 1, no plugin provider, and Redis vector storex-bf-cache-keyextra_fields.cache_debug.hit_type = "direct"Breaking Changes
Yes
This PR enforces the existing semantic cache contract more consistently across setup paths. In practice:
dimension1536fortext-embedding-3-smalldimension: 1and omitproviderSecurity Considerations
No security impact. This change affects configuration validation, startup behavior, UI contract alignment, and documentation only.
Checklist