Allow configuring a default cache key at the plugin level#1729
Open
Georgehe4 wants to merge 2 commits intomaximhq:mainfrom
Open
Allow configuring a default cache key at the plugin level#1729Georgehe4 wants to merge 2 commits intomaximhq:mainfrom
Georgehe4 wants to merge 2 commits intomaximhq:mainfrom
Conversation
requests without an explicit x-bf-cache-key header are automatically cached. When unset (empty string), existing behavior is preserved: caching is opt-in per request.
Georgehe4
changed the title
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a configurable Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Bifrost as "Bifrost/Request Handler"
participant SemanticCache as "semanticcache Plugin"
participant Cache as "Cache Store"
participant LLM as "LLM"
Client->>Bifrost: Send request (maybe no x-bf-cache-key)
Bifrost->>SemanticCache: PreLLMHook(ctx, req)
SemanticCache->>SemanticCache: determine cache_key = ctx.key || DefaultCacheKey
alt cache_key set
SemanticCache->>Cache: Lookup(cache_key)
alt cache hit
Cache-->>SemanticCache: cached response
SemanticCache-->>Bifrost: return cached response
Bifrost-->>Client: cached response
else cache miss
SemanticCache->>LLM: forward request
LLM-->>SemanticCache: response
SemanticCache->>Cache: Store(cache_key, response)
SemanticCache-->>Bifrost: response
Bifrost-->>Client: response
end
else no cache_key
SemanticCache->>LLM: forward request (no caching)
LLM-->>SemanticCache: response
SemanticCache-->>Bifrost: response
Bifrost-->>Client: response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/semanticcache/main.go (1)
39-107:⚠️ Potential issue | 🔴 Critical
DefaultCacheKeyis never populated byUnmarshalJSON— entire feature is dead at runtime.
Config.UnmarshalJSONuses an internalTempConfigto avoid infinite recursion; it then manually copies every field toc.DefaultCacheKeyis missing from bothTempConfigand the copy block, so after JSON unmarshalingc.DefaultCacheKeyis always"". The checks on lines 395 and 539 (plugin.config.DefaultCacheKey != "") therefore never execute, and the feature is completely non-functional when loaded from any JSON/YAML/Helm config.🐛 Proposed fix
type TempConfig struct { Provider string `json:"provider"` Keys []schemas.Key `json:"keys"` EmbeddingModel string `json:"embedding_model,omitempty"` CleanUpOnShutdown bool `json:"cleanup_on_shutdown,omitempty"` Dimension int `json:"dimension"` TTL interface{} `json:"ttl,omitempty"` Threshold float64 `json:"threshold,omitempty"` VectorStoreNamespace string `json:"vector_store_namespace,omitempty"` + DefaultCacheKey string `json:"default_cache_key,omitempty"` ConversationHistoryThreshold int `json:"conversation_history_threshold,omitempty"` CacheByModel *bool `json:"cache_by_model,omitempty"` CacheByProvider *bool `json:"cache_by_provider,omitempty"` ExcludeSystemPrompt *bool `json:"exclude_system_prompt,omitempty"` }And in the manual copy block:
c.VectorStoreNamespace = temp.VectorStoreNamespace c.ConversationHistoryThreshold = temp.ConversationHistoryThreshold +c.DefaultCacheKey = temp.DefaultCacheKey c.Threshold = temp.Threshold🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/main.go` around lines 39 - 107, The TempConfig used by Config.UnmarshalJSON is missing the DefaultCacheKey field and you never copy it into c, so populate TempConfig with DefaultCacheKey string `json:"default_cache_key,omitempty"` and add c.DefaultCacheKey = temp.DefaultCacheKey in the manual copy block inside Config.UnmarshalJSON (near where you set Provider/Keys/EmbeddingModel/etc.) so DefaultCacheKey is correctly preserved when unmarshaling.
🧹 Nitpick comments (1)
plugins/semanticcache/main.go (1)
538-543: Add aDebuglog whenDefaultCacheKeyis applied inPostLLMHook, consistent withPreLLMHook.
PreLLMHook(line 397) emits a debug log when it falls back toDefaultCacheKey;PostLLMHookis silent. Without the log it's harder to diagnose cache-miss vs. no-key scenarios in production traces.📝 Proposed fix
if !ok || cacheKey == "" { if plugin.config.DefaultCacheKey != "" { cacheKey = plugin.config.DefaultCacheKey + plugin.logger.Debug(PluginLoggerPrefix + " Using default cache key: " + cacheKey) } else { return res, nil, nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/main.go` around lines 538 - 543, PostLLMHook currently falls back to plugin.config.DefaultCacheKey when cacheKey is empty but doesn't log this; add a debug log (matching the style used in PreLLMHook) at the point where cacheKey is set to plugin.config.DefaultCacheKey so operators can see when the default key is applied; reference the PostLLMHook function and ensure the log message mirrors PreLLMHook's debug text for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/semanticcache/main.go`:
- Around line 39-107: The TempConfig used by Config.UnmarshalJSON is missing the
DefaultCacheKey field and you never copy it into c, so populate TempConfig with
DefaultCacheKey string `json:"default_cache_key,omitempty"` and add
c.DefaultCacheKey = temp.DefaultCacheKey in the manual copy block inside
Config.UnmarshalJSON (near where you set Provider/Keys/EmbeddingModel/etc.) so
DefaultCacheKey is correctly preserved when unmarshaling.
---
Nitpick comments:
In `@plugins/semanticcache/main.go`:
- Around line 538-543: PostLLMHook currently falls back to
plugin.config.DefaultCacheKey when cacheKey is empty but doesn't log this; add a
debug log (matching the style used in PreLLMHook) at the point where cacheKey is
set to plugin.config.DefaultCacheKey so operators can see when the default key
is applied; reference the PostLLMHook function and ensure the log message
mirrors PreLLMHook's debug text for consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/semanticcache/plugin_default_cache_key_test.go (2)
35-35: Preferbifrost.Ptr()over&forBifrostResponsestruct literals.All five assertion call sites use
&schemas.BifrostResponse{...}. Per the project convention,bifrost.Ptr()should be used consistently instead of the address operator.♻️ Example fix (apply to all five occurrences)
- AssertNoCacheHit(t, &schemas.BifrostResponse{ChatResponse: response1}) + AssertNoCacheHit(t, bifrost.Ptr(schemas.BifrostResponse{ChatResponse: response1}))Based on learnings: "prefer using
bifrost.Ptr()to create pointers instead of the address operator (&) even when&would be valid syntactically… Replace occurrences of&valuewhere a*Tis expected withbifrost.Ptr(value)."Also applies to: 49-49, 83-83, 106-106, 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/plugin_default_cache_key_test.go` at line 35, Replace uses of the address operator for BifrostResponse struct literals with the project helper function: instead of &schemas.BifrostResponse{...} wrap the struct literal with bifrost.Ptr(...). Update all five assertion call sites (e.g. the AssertNoCacheHit/AssertCacheHit invocations that currently pass &schemas.BifrostResponse{...}) to pass bifrost.Ptr(schemas.BifrostResponse{...}) so the code uses bifrost.Ptr and the schemas.BifrostResponse type consistently.
64-84:TestDefaultCacheKey_PerRequestKeyOverridesDefaultonly verifies the miss, not the hit — test can pass with caching entirely disabled.The test caches with the default key, then asserts that a different per-request key is a miss. But it never asserts that a repeat request with the same default key would be a hit. If caching is completely broken, the override key also trivially misses and the test passes vacuously. Add an intermediate same-default-key request after
WaitForCacheto confirm the cache is actually populated before testing the override:♻️ Proposed strengthened assertion
WaitForCache() + // Confirm the default key itself hits the cache before testing override + ctx1b := schemas.NewBifrostContext(context.Background(), schemas.NoDeadline) + response1b, err1b := setup.Client.ChatCompletionRequest(ctx1b, testRequest) + if err1b != nil { + if err1b.Error != nil { + t.Fatalf("Baseline cache-hit request failed: %v", err1b.Error.Message) + } + t.Fatalf("Baseline cache-hit request failed: %v", err1b) + } + AssertCacheHit(t, &schemas.BifrostResponse{ChatResponse: response1b}, string(CacheTypeDirect)) + // Same request but with a DIFFERENT per-request key — should miss🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/plugin_default_cache_key_test.go` around lines 64 - 84, After the initial request using the default key (schemas.NewBifrostContext(...)) and WaitForCache(), add a repeat request using the same default context (call setup.Client.ChatCompletionRequest with a new ctxDefault2 created by schemas.NewBifrostContext) and assert that this second default-key response is a cache hit (use AssertCacheHit with the returned response) before running the existing override-key request and AssertNoCacheHit; this ensures the cache was actually populated prior to testing that CreateContextWithCacheKey("override-key") produces a miss.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/semanticcache/plugin_default_cache_key_test.go`:
- Around line 26-28: The early checks that do "if err1 != nil { return }" after
the first ChatCompletionRequest silently abort the test and mask failures;
change those to either fail the test with t.Fatalf (e.g., t.Fatalf("first
ChatCompletionRequest failed: %v", err1)) or explicitly skip with t.Skipf when
external infra is unavailable. Locate the three occurrences that check err1
after the first ChatCompletionRequest call in plugin_default_cache_key_test.go
(the same pattern appears twice more later) and replace the silent return with a
t.Fatalf or t.Skipf consistent with the test's intent.
---
Nitpick comments:
In `@plugins/semanticcache/plugin_default_cache_key_test.go`:
- Line 35: Replace uses of the address operator for BifrostResponse struct
literals with the project helper function: instead of
&schemas.BifrostResponse{...} wrap the struct literal with bifrost.Ptr(...).
Update all five assertion call sites (e.g. the AssertNoCacheHit/AssertCacheHit
invocations that currently pass &schemas.BifrostResponse{...}) to pass
bifrost.Ptr(schemas.BifrostResponse{...}) so the code uses bifrost.Ptr and the
schemas.BifrostResponse type consistently.
- Around line 64-84: After the initial request using the default key
(schemas.NewBifrostContext(...)) and WaitForCache(), add a repeat request using
the same default context (call setup.Client.ChatCompletionRequest with a new
ctxDefault2 created by schemas.NewBifrostContext) and assert that this second
default-key response is a cache hit (use AssertCacheHit with the returned
response) before running the existing override-key request and AssertNoCacheHit;
this ensures the cache was actually populated prior to testing that
CreateContextWithCacheKey("override-key") produces a miss.
Comment on lines
+26
to
+28
| if err1 != nil { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Silent return on first-request error masks failures across all three tests.
All three tests share the same pattern: if the first ChatCompletionRequest call returns a non-nil error, the test function silently returns. The test reports as passed with nothing verified. This contrasts with the second-request error handling in each test, which correctly calls t.Fatalf. If the embedding service or LLM backend is unavailable, CI will stay green while no assertion is ever executed.
Replace with t.Fatalf (hard fail) or t.Skipf (intentional infrastructure skip) consistently:
🐛 Proposed fix (applied to all three occurrences)
- if err1 != nil {
- return
- }
+ if err1 != nil {
+ if err1.Error != nil {
+ t.Fatalf("First request failed: %v", err1.Error.Message)
+ }
+ t.Fatalf("First request failed: %v", err1)
+ }Also applies to: 66-69, 101-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/semanticcache/plugin_default_cache_key_test.go` around lines 26 - 28,
The early checks that do "if err1 != nil { return }" after the first
ChatCompletionRequest silently abort the test and mask failures; change those to
either fail the test with t.Fatalf (e.g., t.Fatalf("first ChatCompletionRequest
failed: %v", err1)) or explicitly skip with t.Skipf when external infra is
unavailable. Locate the three occurrences that check err1 after the first
ChatCompletionRequest call in plugin_default_cache_key_test.go (the same pattern
appears twice more later) and replace the silent return with a t.Fatalf or
t.Skipf consistent with the test's intent.
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
1 participant
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
Enable default caching
Changes
Type of change
Affected areas
How to test
Set default_cache_key
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines