refactor: semantic cache plugin#3210
Open
Pratham-Mishra04 wants to merge 1 commit intodevfrom
Open
Conversation
|
|
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds cache-hit-type filtering, switches cache-clear to storage cache IDs, and refactors the semantic-cache plugin end-to-end: deterministic direct IDs, per-request cacheState with cleanup, streaming accumulation/replay rewrite, deterministic hashing/metadata, TTL parsing, tests, UI/schema, and docs updates. ChangesEnd-to-end Cache-Hit Filtering & Logs
Semantic Cache Plugin Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant Plugin as SemanticCache Plugin
participant Embedding as Embedding Executor
participant Store as Vector/Chunk Store
Client->>Plugin: Request (PreLLMHook)
Plugin->>Plugin: resolve cache_key, paramsHash, create cacheState
alt direct lookup
Plugin->>Store: GetChunk(directCacheID)
Store-->>Plugin: chunk / miss
else semantic lookup
Plugin->>Embedding: generate embedding
Embedding-->>Plugin: embedding + inputTokens
Plugin->>Store: GetNearest(filters: cache_key, params_hash, plugin_marker, maybe provider/model)
Store-->>Plugin: nearest result / miss
end
Plugin-->>Client: short-circuit response if hit
Client->>Plugin: Upstream provider result (PostLLMHook)
Plugin->>Plugin: shouldSkipCaching? resolve storageID & embedding-write eligibility
Plugin->>Store: async Add (stream or non-stream) with unified metadata
Store-->>Plugin: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Confidence Score: 3/5The refactor is broad and well-structured, but Cleanup now panics on a second call and cacheStates from cache-hit requests still leak until the 60-minute periodic reaper fires. The new Cleanup implementation closes plugin.stopCh unconditionally; Go panics with 'close of closed channel' on a second call. The old implementation only called waitGroup.Wait(), which is safe to invoke any number of times. Any lifecycle framework, graceful-restart path, or test harness that calls Cleanup twice will crash the process. plugins/semanticcache/main.go — Cleanup double-close panic and cache-hit cacheState leak. plugins/semanticcache/utils.go — PostLLMHook provider/model derived from post-fallback extraFields rather than the originally-requested provider. Important Files Changed
Reviews (6): Last reviewed commit: "fix: semantic cache fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/config/views/pluginsForm.tsx (1)
249-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow disabling an already-enabled plugin even when no embedding providers exist.
Line 249 disables the toggle when
embeddingProviders.length === 0, which also blocks turning the plugin OFF if it is currently enabled.Proposed fix
- disabled={!isVectorStoreEnabled || providersLoading || embeddingProviders.length === 0} + disabled={!isVectorStoreEnabled || providersLoading || (embeddingProviders.length === 0 && !originalCacheEnabled)}🤖 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 249 - 253, The toggle is currently disabled whenever embeddingProviders.length === 0, which prevents turning an already-enabled plugin off; change the disabled prop to only block enabling (e.g., disabled={providersLoading || (!isVectorStoreEnabled && embeddingProviders.length === 0)}) and add a guard in onCheckedChange to ignore attempts to enable when embeddingProviders.length === 0 (e.g., if (checked && embeddingProviders.length === 0) return;) before calling handleSemanticCacheToggle(checked), leaving isVectorStoreEnabled, providersLoading, embeddingProviders, onCheckedChange, and handleSemanticCacheToggle as the referenced symbols to locate and update.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/middlewares.go (1)
52-79: ⚡ Quick winRemove the now-dead skip/goto branch in
CorsMiddleware.After disabling request-completion logging, this block is a no-op: the skip-path check and
goto corsFlowno longer change behavior. Please delete the dead control-flow and commented logger block to keep this middleware readable.♻️ Suggested cleanup
- // startTime := time.Now() - // skip logging if it's a /health check request - if slices.IndexFunc(loggingSkipPaths, func(path string) bool { - return strings.HasPrefix(string(ctx.RequestURI()), path) - }) != -1 { - goto corsFlow - } - // defer func() { - // ... - // }() - corsFlow: + // Request-completion logging intentionally disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/middlewares.go` around lines 52 - 79, In CorsMiddleware, remove the dead skip-path check and goto along with the entirely commented-out deferred request-completion logging block: delete the slices.IndexFunc(...) check referencing loggingSkipPaths and the goto corsFlow label, remove the commented defer block that builds the request log (including references to startTime, logger.LogHTTPRequest and schemas.* fields), and remove the remaining corsFlow: label so the middleware has no dead control-flow or commented legacy code.ui/app/workspace/config/views/pluginsForm.tsx (1)
324-332: ⚡ Quick winAdd a test selector to the new embedding model control.
The new
ModelMultiselectis interactive but has nodata-testid, which makes E2E targeting inconsistent.Proposed fix
<ModelMultiselect inputId="embedding_model" + data-testid="semantic-cache-embedding-model-multiselect" isSingleSelect provider={cacheConfig.provider || undefined} value={cacheConfig.embedding_model ?? ""}As per coding guidelines: "Add data-testid to all new interactive elements in React components for E2E test compatibility" and use
data-testid="<entity>-<element>-<qualifier>".🤖 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 324 - 332, The ModelMultiselect instance rendering the embedding model picker lacks a data-testid; add a data-testid prop to the component (ModelMultiselect) using the project's naming convention "<entity>-<element>-<qualifier>" so E2E tests can target it reliably (e.g., use something like data-testid="cacheconfig-embedding_model-input" or similar consistent name), leaving the other props (inputId, provider, value, onChange, placeholder, disabled) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/rdb.go`:
- Around line 195-217: The code currently skips adding any predicate when
filters.CacheHitTypes is non-empty but none pass the allowlist, causing
unfiltered results; change the branch so that after building valid you check if
len(filters.CacheHitTypes) > 0 && len(valid) == 0 and in that case add a safe
always-false predicate to baseQuery (e.g. baseQuery = baseQuery.Where("1=0") or
dialect-appropriate FALSE) so queries like cache_hit_types=foo return no rows;
update the block around filters.CacheHitTypes, baseQuery, and the Dialector
check (and keep the existing regex/json extraction logic unchanged when valid is
non-empty).
In `@plugins/semanticcache/main.go`:
- Around line 710-724: ClearCacheForRequestID currently deletes using the raw
requestID but new entries use deterministic directCacheID via
resolveStorageIDAndEmbedding, so callers passing
schemas.BifrostContextKeyRequestID miss entries; update
Plugin.ClearCacheForRequestID to accept either a directCacheID or compute the
correct storage ID by calling resolveStorageIDAndEmbedding (or the helper that
produces directCacheID) for the given requestID and then call
plugin.store.Delete(ctx, plugin.config.VectorStoreNamespace, computedStorageID);
ensure logging uses the actual storage key deleted and preserve the
timeout/cancel pattern already present.
- Around line 285-286: WaitForPendingOperations() is blocked by the long-lived
runStreamCleanupLoop because Init increments plugin.waitGroup for that
goroutine; remove the forever-loop from the same wait group used to flush
pending writes. Concretely, stop calling plugin.waitGroup.Add(1) for
runStreamCleanupLoop in Init (or move that Add/Done into a separate
longLivedWaitGroup variable or into Cleanup()), so runStreamCleanupLoop is not
waited on by WaitForPendingOperations(); keep WaitForPendingOperations() waiting
only for short-lived write/flush goroutines so tests can call it to flush
pending cache writes.
In `@plugins/semanticcache/plugin_cache_type_test.go`:
- Around line 617-619: The tests still call the removed 3-argument
performDirectSearch; update the remaining call sites in
TestDefaultDirectSearchSetsStorageIDForDeterministicWrites and
TestPerformDirectSearchDisablesScanFallbackForLegacyLookup (also the calls
around lines 858-860) to match the new performDirectSearch signature in
plugins/semanticcache/search.go by adding the new parameter(s) and adapting to
any changed return values; ensure you pass the appropriate
context/req/plugin/storageID or options values that the new signature requires
and update the assertions to reflect the new outputs.
In `@plugins/semanticcache/stream.go`:
- Around line 62-70: The reaper currently uses the first chunk timestamp
(Chunks[0].Timestamp) to determine staleness, causing active streams to be
removed; update the StreamAccumulator struct to include a lastSeen time field,
set/update that field inside addStreamChunk (and when creating the accumulator),
and change the reaping logic (the code referenced around lines 141-150) to
compare streamAccumulatorMaxAge against accumulator.lastSeen instead of
Chunks[0].Timestamp so last activity, not the first chunk, controls expiry.
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Around line 126-134: The effect that runs when embeddingProviders changes
currently forces provider to embeddingProviders[0].name and can overwrite a
user's in-progress selection; change the setCacheConfig call inside the effect
so it only sets the default provider when there is no existing provider (i.e.,
when prev.provider is null/undefined/empty) rather than unconditionally—inside
the setCacheConfig updater for the effect referencing embeddingProviders and
semanticCachePlugin?.config, short-circuit and return prev if prev.provider
already exists, otherwise populate provider with embeddingProviders[0].name and
fill embedding_model/dimension defaults.
---
Outside diff comments:
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Around line 249-253: The toggle is currently disabled whenever
embeddingProviders.length === 0, which prevents turning an already-enabled
plugin off; change the disabled prop to only block enabling (e.g.,
disabled={providersLoading || (!isVectorStoreEnabled &&
embeddingProviders.length === 0)}) and add a guard in onCheckedChange to ignore
attempts to enable when embeddingProviders.length === 0 (e.g., if (checked &&
embeddingProviders.length === 0) return;) before calling
handleSemanticCacheToggle(checked), leaving isVectorStoreEnabled,
providersLoading, embeddingProviders, onCheckedChange, and
handleSemanticCacheToggle as the referenced symbols to locate and update.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 52-79: In CorsMiddleware, remove the dead skip-path check and goto
along with the entirely commented-out deferred request-completion logging block:
delete the slices.IndexFunc(...) check referencing loggingSkipPaths and the goto
corsFlow label, remove the commented defer block that builds the request log
(including references to startTime, logger.LogHTTPRequest and schemas.* fields),
and remove the remaining corsFlow: label so the middleware has no dead
control-flow or commented legacy code.
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Around line 324-332: The ModelMultiselect instance rendering the embedding
model picker lacks a data-testid; add a data-testid prop to the component
(ModelMultiselect) using the project's naming convention
"<entity>-<element>-<qualifier>" so E2E tests can target it reliably (e.g., use
something like data-testid="cacheconfig-embedding_model-input" or similar
consistent name), leaving the other props (inputId, provider, value, onChange,
placeholder, disabled) 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dacf00c4-f836-45fb-a591-e2b29e8069ed
📒 Files selected for processing (20)
.gitignorecore/schemas/bifrost.goframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goframework/modelcatalog/sync.goplugins/semanticcache/main.goplugins/semanticcache/plugin_cache_type_test.goplugins/semanticcache/search.goplugins/semanticcache/stream.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/pluginsForm.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/components/filters/logsFilterSidebar.tsxui/lib/constants/logs.tsui/lib/store/apis/logsApi.tsui/lib/types/logs.ts
Pratham-Mishra04
force-pushed
the
04-30-fix_semantic_cache_fixes
branch
from
76e9d94 to
2ee69fd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/semanticcache/plugin_edge_cases_test.go (1)
350-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a unique cache key per subtest here.
All content-variation cases reuse
content-variations-test, and this loop never clears the store between subtests. That lets earlier semantic entries bleed into later cases, so a case may stop validating its own first write path.Suggested fix
- ctx := CreateContextWithCacheKey(t, "content-variations-test") + ctx := CreateContextWithCacheKey(t, "content-variations-test-"+tt.name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/plugin_edge_cases_test.go` around lines 350 - 355, The subtests reuse the same cache key ("content-variations-test") causing state leakage between cases; update the context creation in the test loop so each subtest gets a unique cache key (e.g., derive from tt.name or t.Name(), or append a UUID/timestamp) when calling CreateContextWithCacheKey so each run uses an isolated store and prevents earlier semantic entries from affecting later cases.
🧹 Nitpick comments (5)
plugins/semanticcache/utils.go (1)
5-16: ⚡ Quick winSwitch the cache serialization path to
sonic.These marshal/unmarshal calls are on the cache read/write path, so they should use the repo-standard hot-path JSON library instead of
encoding/json.Suggested fix
- "encoding/json" + sonic "github.com/bytedance/sonic"- responseData, err := json.Marshal(res) + responseData, err := sonic.Marshal(res)- if err := json.Unmarshal([]byte(v), &stringArray); err != nil { + if err := sonic.Unmarshal([]byte(v), &stringArray); err != nil {As per coding guidelines
**/*.go: Use github.com/bytedance/sonic for JSON marshaling in hot paths; use encoding/json for custom marshaling in schemas.Also applies to: 433-445, 495-520
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/utils.go` around lines 5 - 16, Replace the hot-path JSON usage of encoding/json in plugins/semanticcache/utils.go with the repo-standard fast library: remove the "encoding/json" import and add "github.com/bytedance/sonic" to the imports, then change all json.Marshal/json.Unmarshal calls in this file (including the cache read/write sites around the ranges mentioned: roughly lines 433-445 and 495-520) to use sonic.Marshal/sonic.Unmarshal instead; leave any custom schema marshaling that intentionally uses encoding/json (schemas package) unchanged.plugins/semanticcache/plugin_nil_content_test.go (1)
91-92: ⚡ Quick winAssert the nil-content path stays error-free.
This test currently passes even if
extractTextForEmbeddingstarts returning an error. Since the regression you care about is “nil content should be handled safely,” please fail the test whenerr != nil.💡 Tighten the assertion
text, err := plugin.extractTextForEmbedding(nil, tt.request) - t.Logf("text=%q, err=%v", text, err) + if err != nil { + t.Fatalf("extractTextForEmbedding returned error: %v", err) + } + t.Logf("text=%q", text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/plugin_nil_content_test.go` around lines 91 - 92, The test must fail if extractTextForEmbedding returns an error for the nil-content case: after calling plugin.extractTextForEmbedding(nil, tt.request) (symbols: extractTextForEmbedding, plugin, tt.request) assert that err is nil and fail the test when it's not (e.g. use t.Fatalf or require.NoError) before proceeding to inspect text so the nil-content regression is caught.transports/bifrost-http/handlers/cache.go (1)
55-69: 💤 Low valueMinor validation inconsistency with
clearCachehandler.The
clearCacheByKeyhandler validates the type assertion (!ok) but doesn't check for empty string likeclearCachedoes on line 41. Consider adding an empty string check for consistency, though an empty cache key is unlikely to match anything meaningful.♻️ Optional: Add empty string validation for consistency
func (h *CacheHandler) clearCacheByKey(ctx *fasthttp.RequestCtx) { cacheKey, ok := ctx.UserValue("cacheKey").(string) - if !ok { + if !ok || cacheKey == "" { SendError(ctx, fasthttp.StatusBadRequest, "Invalid cache key") 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 `@transports/bifrost-http/handlers/cache.go` around lines 55 - 69, The clearCacheByKey handler currently only type-asserts cacheKey but doesn't reject empty strings; update clearCacheByKey to mirror clearCache by checking cacheKey == "" after the type assertion and call SendError(ctx, fasthttp.StatusBadRequest, "Invalid cache key") if empty, before invoking h.plugin.ClearCacheForKey(cacheKey), leaving SendError and SendJSON calls unchanged.transports/bifrost-http/handlers/cache_test.go (2)
78-89: ⚡ Quick winAssert no plugin call when
cacheIduser value is missing.This test checks status code only; add a call-count assertion to prevent regressions where the handler still invokes clearing logic on invalid input.
Proposed test hardening
func TestClearCache_MissingUserValue(t *testing.T) { clearer := &fakeCacheClearer{} h := &CacheHandler{plugin: clearer} @@ if got := ctx.Response.StatusCode(); got != fasthttp.StatusBadRequest { t.Fatalf("expected 400 when cacheId user value missing, got %d", got) } + if len(clearer.idCalls) != 0 { + t.Fatalf("expected no Clear calls when cacheId is missing, got %v", clearer.idCalls) + } }🤖 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/cache_test.go` around lines 78 - 89, Add an assertion that the fake cache clearer was not invoked when the user value "cacheId" is missing: after calling CacheHandler.clearCache (in TestClearCache_MissingUserValue) assert that the fakeCacheClearer call-count field (e.g., callCount / clearedCalls on the fakeCacheClearer) is zero so the test fails if CacheHandler.clearCache still calls clearer.Clear...; if the fake doesn't already expose a counter, add one to fakeCacheClearer and increment it in its Clear method, then assert it remains 0.
112-139: ⚡ Quick winAdd negative-path coverage for
clearCacheByKey(empty/missing key).
clearCachealready guards invalid path values, butclearCacheByKeytests currently miss equivalent bad-input assertions. Adding them improves contract symmetry and catches accidental plugin calls.Proposed additional tests
+func TestClearCacheByKey_RejectsEmptyKey(t *testing.T) { + clearer := &fakeCacheClearer{} + h := &CacheHandler{plugin: clearer} + + ctx := newCacheCtx("cacheKey", "") + h.clearCacheByKey(ctx) + + if got := ctx.Response.StatusCode(); got != fasthttp.StatusBadRequest { + t.Fatalf("expected 400 for empty key, got %d", got) + } + if len(clearer.keyCalls) != 0 { + t.Fatalf("expected no Clear calls on bad key, got %v", clearer.keyCalls) + } +} + +func TestClearCacheByKey_MissingUserValue(t *testing.T) { + clearer := &fakeCacheClearer{} + h := &CacheHandler{plugin: clearer} + + ctx := &fasthttp.RequestCtx{} + h.clearCacheByKey(ctx) + + if got := ctx.Response.StatusCode(); got != fasthttp.StatusBadRequest { + t.Fatalf("expected 400 when cacheKey user value missing, got %d", got) + } + if len(clearer.keyCalls) != 0 { + t.Fatalf("expected no Clear calls when cacheKey is missing, got %v", clearer.keyCalls) + } +}🤖 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/cache_test.go` around lines 112 - 139, Add a new test that covers the negative path for CacheHandler.clearCacheByKey: create a fakeCacheClearer and CacheHandler as in TestClearCacheByKey_OK, call h.clearCacheByKey with a context made via newCacheCtx using an empty string (or missing) key, assert that ctx.Response.StatusCode() is fasthttp.StatusBadRequest (or the handler's validation status) and assert that the fake clearer's keyCalls slice remains empty (i.e., ClearCacheForKey was not invoked); place this alongside TestClearCacheByKey_OK and TestClearCacheByKey_PluginErrorReturns500 to ensure symmetry of input validation.
🤖 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/schemas/context.go`:
- Around line 153-158: The Root method currently only returns a single level of
valueDelegate which leaves intermediate pooled scopes in the chain; modify
BifrostContext.Root to traverse/unwarp the delegation chain by following
valueDelegate repeatedly until nil (e.g., loop while bc.valueDelegate != nil
updating bc = bc.valueDelegate) and then return the ultimate root; update the
method on type BifrostContext (Root) so it always resolves to the real request
root even when multiple scoped contexts were derived.
In `@plugins/semanticcache/plugin_api_test.go`:
- Around line 352-358: The test currently does a blocking receive <-sc.Stream
before calling ctx.Cancel(), which can hang if the first chunk never arrives;
change that receive to a guarded receive using a timeout (e.g. select between
sc.Stream and time.After or a context with timeout) so the test fails fast if no
first chunk is produced, then call ctx.Cancel() and proceed to the existing
drain/timeout logic; reference the sc.Stream receive and ctx.Cancel() locations
when making the change.
In `@plugins/semanticcache/plugin_core_test.go`:
- Around line 14-16: The test TestSemanticCacheBasicFunctionality is
timing-sensitive and should not run in parallel; remove the t.Parallel() call
(the parallelization invoked in TestSemanticCacheBasicFunctionality) so the test
runs serially and avoids amplified CPU/network contention that makes the ratio
assertion flaky in CI.
In `@plugins/semanticcache/plugin_embedding_test.go`:
- Around line 84-86: The test is supposed to exercise the "no cache key" path
but calling CreateContextWithCacheKey(t, "") still sets CacheKey (derived from
t.Name()); replace that call so the context truly has no CacheKey — e.g. use
context.Background() (or a newly added helper like CreateContextWithoutCacheKey)
in the test instead of CreateContextWithCacheKey(t, "") so the CacheKey field
remains unset and the uncached path is exercised.
In `@plugins/semanticcache/plugin_responses_test.go`:
- Around line 307-308: Replace the inconsistent context creation in the "no
cache key" test by using newBaseTestContext() instead of
CreateContextWithCacheKey(t, ""); locate the test that currently calls
CreateContextWithCacheKey(t, "") and change it to newBaseTestContext() so the
test uses an explicit base (no-key) context and avoids accidentally supplying a
cache key.
In `@plugins/semanticcache/search.go`:
- Around line 342-365: The loop currently unmarshals each chunk (json.Unmarshal
into cachedResponse) and skips malformed ones, which can truncate the stream and
skip stampCacheDebugForHit; instead, pre-validate the entire streamArray before
replay: iterate over all chunkStr entries, attempt json.Unmarshal into a
temporary struct (and ensure ExtraFields/RequestType are present), and if any
unmarshal/validation fails treat it as a cache miss (log the error and abort
replay) so that plugin.stampCacheDebugForHit
(plugin.stampCacheDebugForHit(state, cachedResponse.GetExtraFields(), result.ID,
...)) and downstream final-chunk handling are guaranteed to run only for
fully-valid cached streams.
- Around line 167-179: The switch on embedding currently handles EmbeddingStr,
EmbeddingArray and Embedding2DArray but omits EmbeddingInt8Array and
EmbeddingInt32Array; add cases for embedding.EmbeddingInt8Array and
embedding.EmbeddingInt32Array that convert those numeric arrays to []float32
using helper functions (e.g. toFloat32EmbeddingFromInt8 and
toFloat32EmbeddingFromInt32) and return the converted slice along with
inputTokens and nil error so those formats no longer fall through to the final
error branch.
---
Outside diff comments:
In `@plugins/semanticcache/plugin_edge_cases_test.go`:
- Around line 350-355: The subtests reuse the same cache key
("content-variations-test") causing state leakage between cases; update the
context creation in the test loop so each subtest gets a unique cache key (e.g.,
derive from tt.name or t.Name(), or append a UUID/timestamp) when calling
CreateContextWithCacheKey so each run uses an isolated store and prevents
earlier semantic entries from affecting later cases.
---
Nitpick comments:
In `@plugins/semanticcache/plugin_nil_content_test.go`:
- Around line 91-92: The test must fail if extractTextForEmbedding returns an
error for the nil-content case: after calling
plugin.extractTextForEmbedding(nil, tt.request) (symbols:
extractTextForEmbedding, plugin, tt.request) assert that err is nil and fail the
test when it's not (e.g. use t.Fatalf or require.NoError) before proceeding to
inspect text so the nil-content regression is caught.
In `@plugins/semanticcache/utils.go`:
- Around line 5-16: Replace the hot-path JSON usage of encoding/json in
plugins/semanticcache/utils.go with the repo-standard fast library: remove the
"encoding/json" import and add "github.com/bytedance/sonic" to the imports, then
change all json.Marshal/json.Unmarshal calls in this file (including the cache
read/write sites around the ranges mentioned: roughly lines 433-445 and 495-520)
to use sonic.Marshal/sonic.Unmarshal instead; leave any custom schema marshaling
that intentionally uses encoding/json (schemas package) unchanged.
In `@transports/bifrost-http/handlers/cache_test.go`:
- Around line 78-89: Add an assertion that the fake cache clearer was not
invoked when the user value "cacheId" is missing: after calling
CacheHandler.clearCache (in TestClearCache_MissingUserValue) assert that the
fakeCacheClearer call-count field (e.g., callCount / clearedCalls on the
fakeCacheClearer) is zero so the test fails if CacheHandler.clearCache still
calls clearer.Clear...; if the fake doesn't already expose a counter, add one to
fakeCacheClearer and increment it in its Clear method, then assert it remains 0.
- Around line 112-139: Add a new test that covers the negative path for
CacheHandler.clearCacheByKey: create a fakeCacheClearer and CacheHandler as in
TestClearCacheByKey_OK, call h.clearCacheByKey with a context made via
newCacheCtx using an empty string (or missing) key, assert that
ctx.Response.StatusCode() is fasthttp.StatusBadRequest (or the handler's
validation status) and assert that the fake clearer's keyCalls slice remains
empty (i.e., ClearCacheForKey was not invoked); place this alongside
TestClearCacheByKey_OK and TestClearCacheByKey_PluginErrorReturns500 to ensure
symmetry of input validation.
In `@transports/bifrost-http/handlers/cache.go`:
- Around line 55-69: The clearCacheByKey handler currently only type-asserts
cacheKey but doesn't reject empty strings; update clearCacheByKey to mirror
clearCache by checking cacheKey == "" after the type assertion and call
SendError(ctx, fasthttp.StatusBadRequest, "Invalid cache key") if empty, before
invoking h.plugin.ClearCacheForKey(cacheKey), leaving SendError and SendJSON
calls 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9762ab5e-8d60-4d60-b04f-6edf04c58a4a
📒 Files selected for processing (53)
.claude/skills/docs-writer/SKILL.md.gitignorecore/schemas/bifrost.gocore/schemas/context.godocs/features/semantic-caching.mdxdocs/migration-guides/v1.5.0.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/cache.yamlframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goframework/modelcatalog/sync.goframework/vectorstore/weaviate.goplugins/logging/main.goplugins/logging/operations.goplugins/semanticcache/main.goplugins/semanticcache/main_test.goplugins/semanticcache/plugin_api_test.goplugins/semanticcache/plugin_cache_type_test.goplugins/semanticcache/plugin_conversation_config_test.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_cross_cache_test.goplugins/semanticcache/plugin_default_cache_key_test.goplugins/semanticcache/plugin_edge_cases_test.goplugins/semanticcache/plugin_embedding_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/plugin_nil_content_test.goplugins/semanticcache/plugin_no_store_test.goplugins/semanticcache/plugin_normalization_test.goplugins/semanticcache/plugin_paths_test.goplugins/semanticcache/plugin_responses_test.goplugins/semanticcache/plugin_streaming_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/search.goplugins/semanticcache/state.goplugins/semanticcache/stream.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/cache_test.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/pluginsForm.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/components/filters/logsFilterSidebar.tsxui/lib/constants/logs.tsui/lib/store/apis/logsApi.tsui/lib/types/config.tsui/lib/types/logs.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- plugins/logging/operations.go
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- ui/lib/types/logs.ts
- transports/bifrost-http/handlers/logging.go
- core/schemas/bifrost.go
- .claude/skills/docs-writer/SKILL.md
- transports/bifrost-http/handlers/middlewares.go
🚧 Files skipped from review as they are similar to previous changes (10)
- ui/lib/constants/logs.ts
- ui/app/workspace/logs/page.tsx
- framework/logstore/matviews.go
- ui/components/filters/logsFilterSidebar.tsx
- ui/app/workspace/logs/sheets/logDetailView.tsx
- framework/modelcatalog/sync.go
- ui/lib/store/apis/logsApi.ts
- framework/logstore/tables.go
- plugins/semanticcache/stream.go
- plugins/semanticcache/main.go
Comment on lines
342
to
+365
| if err := json.Unmarshal([]byte(chunkStr), &cachedResponse); err != nil { | ||
| plugin.logger.Warn("%s Failed to unmarshal stream chunk %d, skipping: %v", PluginLoggerPrefix, i, err) | ||
| plugin.logger.Warn("Failed to unmarshal stream chunk %d, skipping: %v", i, err) | ||
| continue | ||
| } | ||
|
|
||
| // Ensure RequestType is set on every chunk so downstream consumers | ||
| // (logging, telemetry, etc.) correctly identify this as a streaming response. | ||
| // (logging, telemetry) correctly identify this as a streaming response. | ||
| if ef := cachedResponse.GetExtraFields(); ef != nil && ef.RequestType == "" { | ||
| ef.RequestType = req.RequestType | ||
| } | ||
|
|
||
| // Add cache debug to only the last chunk | ||
| if i == len(streamArray)-1 { | ||
| ctx.SetValue(schemas.BifrostContextKeyStreamEndIndicator, true) | ||
| extraFields := cachedResponse.GetExtraFields() | ||
| cacheDebug := schemas.BifrostCacheDebug{ | ||
| CacheHit: true, | ||
| HitType: bifrost.Ptr(string(cacheType)), | ||
| CacheID: bifrost.Ptr(result.ID), | ||
| RequestedProvider: bifrost.Ptr(string(requestedProvider)), | ||
| RequestedModel: bifrost.Ptr(requestedModel), | ||
| } | ||
| if cacheType == CacheTypeSemantic { | ||
| cacheDebug.ProviderUsed = bifrost.Ptr(string(plugin.config.Provider)) | ||
| cacheDebug.ModelUsed = bifrost.Ptr(plugin.config.EmbeddingModel) | ||
| cacheDebug.Threshold = &threshold | ||
| cacheDebug.Similarity = &similarity | ||
| cacheDebug.InputTokens = &inputTokens | ||
| } else { | ||
| cacheDebug.ProviderUsed = nil | ||
| cacheDebug.ModelUsed = nil | ||
| cacheDebug.Threshold = nil | ||
| cacheDebug.Similarity = nil | ||
| cacheDebug.InputTokens = nil | ||
| } | ||
| extraFields.CacheDebug = &cacheDebug | ||
| // stampCacheDebugForHit marks this chunk as the cache-hit final | ||
| // chunk; cache.PostLLMHook keys off CacheDebug.CacheHit=true to | ||
| // set BifrostContextKeyStreamEndIndicator on the root ctx | ||
| // synchronously (same goroutine as logging.PostLLMHook). | ||
| // | ||
| // We deliberately do NOT call ctx.Root().SetValue here. Doing | ||
| // so races against the receiver's PostLLMHook for the previous | ||
| // chunk: the cache replay can advance to iteration N (and | ||
| // write the indicator) while the receiver is still running | ||
| // PostLLMHooks for chunk N-1, poisoning that chunk's | ||
| // IsFinalChunk read and causing duplicate "final" events. | ||
| plugin.stampCacheDebugForHit(state, cachedResponse.GetExtraFields(), result.ID, requestedProvider, requestedModel, cacheType, threshold, similarity, inputTokens) |
Contributor
There was a problem hiding this comment.
Fail closed on malformed cached stream chunks.
Skipping a bad chunk here turns a corrupt cache entry into a truncated stream. If the malformed chunk is the final one, stampCacheDebugForHit never runs, so downstream final-chunk handling and log finalization can be missed entirely. Treat any decode failure as a cache miss by validating the full chunk list before starting replay.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/semanticcache/search.go` around lines 342 - 365, The loop currently
unmarshals each chunk (json.Unmarshal into cachedResponse) and skips malformed
ones, which can truncate the stream and skip stampCacheDebugForHit; instead,
pre-validate the entire streamArray before replay: iterate over all chunkStr
entries, attempt json.Unmarshal into a temporary struct (and ensure
ExtraFields/RequestType are present), and if any unmarshal/validation fails
treat it as a cache miss (log the error and abort replay) so that
plugin.stampCacheDebugForHit (plugin.stampCacheDebugForHit(state,
cachedResponse.GetExtraFields(), result.ID, ...)) and downstream final-chunk
handling are guaranteed to run only for fully-valid cached streams.
Collaborator
Author
There was a problem hiding this comment.
This is intentional and documented inline (search.go:344-348): pre-decoding the entire chunk list adds O(N) latency before the first chunk is delivered, which defeats the purpose of streaming long responses. A malformed chunk is extremely unlikely in practice — we wrote the JSON ourselves — and on the rare occasion it happens, log+skip is preferred over blocking the cache hit entirely. The trade-off is deliberate.
Contributor
There was a problem hiding this comment.
@Pratham-Mishra04, understood — the O(N) pre-decode penalty on long streams is a real cost, the JSON origin makes corruption extremely unlikely, and the inline comment at lines 344–348 already documents the rationale. The log+skip trade-off is well-reasoned. I'll note this for future reviews.
✏️ Learnings added
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3210
File: plugins/semanticcache/search.go:353-376
Timestamp: 2026-05-08T20:11:46.826Z
Learning: In maximhq/bifrost's semantic cache plugin (`plugins/semanticcache/search.go`), the streaming cache replay goroutine (`buildStreamingResponseFromResult`) deliberately does NOT pre-validate all stream chunks before starting replay. Each chunk is unmarshalled inline and any decode failure is logged and skipped (not treated as a cache miss). This is an intentional trade-off: pre-decoding the full list would add O(N) latency before the first chunk is delivered (defeating the purpose of streaming), and malformed chunks are extremely unlikely in practice because the plugin wrote the JSON itself. Do not flag this pattern or suggest pre-validation of stream chunks in this function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1312
File: framework/modelcatalog/pricing.go:276-426
Timestamp: 2026-01-14T04:40:11.480Z
Learning: In the Bifrost codebase, ImageUsage and other usage types guarantee that TotalTokens is populated (computed as InputTokens + OutputTokens if providers don’t supply TotalTokens). Reviewers can rely on this invariant and should not assume TotalTokens may be missing when input/output tokens exist. When implementing tiering logic or token-based decisions, you can safely use TotalTokens without extra null/zero guards, provided you’re in a context where InputTokens and OutputTokens are present. If a branch might discard tokens, ensure the invariant is preserved or add explicit checks only where the inputs are confirmed to be valid.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1326
File: plugins/semanticcache/utils.go:11-11
Timestamp: 2026-01-14T10:34:23.134Z
Learning: In files under plugins/semanticcache (e.g., utils.go and similar), when req.RequestType is ImageGenerationRequest or ImageGenerationStreamRequest, the corresponding typed field (req.ImageGenerationRequest) is guaranteed non-nil by the framework. Do not insert nil checks for these fields in these cases. This pattern should apply to all similar files in that directory; for other request types, keep appropriate nil checks as needed. Consider adding unit tests to verify non-nil guarantees and refactor accordingly.
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1326
File: plugins/semanticcache/test_utils.go:545-559
Timestamp: 2026-01-14T13:30:28.760Z
Learning: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability. Replace occurrences of &value where a *T is expected with bifrost.Ptr(value) (or an equivalent call) and ensure the function is in scope and used correctly for the target pointer type.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1352
File: plugins/litellmcompat/main.go:76-89
Timestamp: 2026-01-18T07:00:26.487Z
Learning: In the Bifrost plugin system, disabled plugins are not added to the plugin chain; they are loaded/unloaded via ReloadPlugin/RemovePlugin based on configuration. Therefore, internal checks for an Enabled flag inside PreHook or PostHook are unnecessary, as these methods will not be invoked for disabled plugins. Reviewers should ensure code relies on the loader to exclude disabled plugins and consider removing redundant Enabled checks in plugin methods. This guideline applies to all Go files under the plugins directory.
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 1740
File: transports/bifrost-http/handlers/governance.go:3168-3214
Timestamp: 2026-02-23T07:58:44.087Z
Learning: In this codebase using GORM, models with CreatedAt and UpdatedAt fields of type time.Time tagged with gorm:"autoCreateTime" and gorm:"autoUpdateTime" are populated automatically by GORM on insert/update. Do not manually set them with time.Now(). Remove any manual initialization; rely on GORM's automatic timestamps. This applies to all Go files with GORM models in the repository.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1670
File: core/providers/anthropic/anthropic.go:689-707
Timestamp: 2026-02-24T04:21:32.824Z
Learning: In Go streaming handlers that reuse pooled response objects (e.g., BifrostChatResponse, BifrostResponsesResponse.Response) via ProcessAndSendResponse, do not release them back to their pools while asynchronous readers (PostLLMHook goroutines) may still access them. Releasing between Acquire and use can cause data races and panics when fields are read by the goroutines. Rely on GC after all references are dropped, and apply this safety pattern to all pooled response types passed through ProcessAndSendResponse in streaming contexts. This should be documented and enforced consistently across all relevant Go files.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1836
File: core/providers/utils/utils.go:1994-2019
Timestamp: 2026-03-01T13:11:33.245Z
Learning: Enforce the repository-wide convention: all object pools must use raw sync.Pool (not pool.New[T]() or generic pool builders). When reviewing any Go files, verify that pooling code uses sync.Pool directly and constistent with the examples in maximhq/bifrost (e.g., core/bifrost.go, core/providers/anthropic/anthropic.go, core/providers/cohere/cohere.go, core/schemas/plugin.go, framework/tracing/store.go). In particular, do not introduce pool.New[T]() usage; ensure existing pool implementations remain the raw sync.Pool pattern.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2152
File: framework/logstore/tables.go:422-431
Timestamp: 2026-03-18T09:04:27.884Z
Learning: Do not flag usages of new(expr) in Go code as compile-time errors. Starting with Go 1.26, new() accepts an expression operand (e.g., new(string(data))), and is valid syntax. Reviewers should only flag actual invalid uses per the Go version used in CI, and assume new(expr) forms are allowed across Go files.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2322
File: core/providers/anthropic/responses.go:1988-1992
Timestamp: 2026-03-27T09:20:29.538Z
Learning: In maximhq/bifrost, `BifrostContext` (`core/schemas/context.go`) is a mutable shared context. Its `(*BifrostContext).SetValue(key, value any)` is a pointer-receiver that mutates the internal `userValues map[any]any` in place under a write mutex; it does not create a derived context like Go’s `context.WithValue`. Therefore, when reviewing code, do not flag `SetValue` usage as failing to “propagate” context—subsequent `ctx.Value()` reads on the same `*BifrostContext` pointer should see the updated value immediately.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2339
File: plugins/logging/utils.go:531-543
Timestamp: 2026-03-31T05:42:40.984Z
Learning: When reviewing Go code that uses `schemas.ResponsesMessageContent` (as in `plugins/logging/utils.go`), treat `ContentStr` and `ContentBlocks` as mutually exclusive content sources. The type’s `MarshalJSON` enforces that: if `ContentStr != nil`, it is the sole content source and code should not include or suggest a fallback-to-`ContentBlocks` guard when `ContentStr` is non-nil (even if it might be empty). Conversely, only use `ContentBlocks` when `ContentStr` is nil, per the schema contract.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2339
File: plugins/logging/main.go:740-744
Timestamp: 2026-04-03T10:46:22.677Z
Learning: In maximhq/bifrost, Bifrost context key/value assignments are done with `(*BifrostContext).SetValue(key, value)`, not with `context.WithValue`. During code review/searches for where `BifrostContextKey*` constants are set, look for `.SetValue(` patterns (e.g., `rg -n 'SetValue.*BifrostContextKey'`) rather than `context.WithValue`/`WithValue.*Key` to avoid false findings that a key is never set.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2510
File: framework/objectstore/gcs.go:32-43
Timestamp: 2026-04-04T10:05:42.632Z
Learning: In maximhq/bifrost, `schemas.EnvVar` does not implement `IsDefined()`. Reviewers should not flag or suggest calling `schemas.EnvVar.IsDefined()` anywhere in the repository. To check whether an EnvVar config is both present and resolved, use `cfg.Field != nil && cfg.Field.GetValue() != ""` (where `cfg.Field` is the `*schemas.EnvVar` pointer field being evaluated).
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2509
File: framework/logstore/store.go:110-127
Timestamp: 2026-04-04T10:30:13.550Z
Learning: In maximhq/bifrost, do not reference `EnvVar.IsDefined()` (it does not exist on `schemas.EnvVar`). To validate a non-pointer `schemas.EnvVar` field, check `field.GetValue() == ""` (for “defined” it should be non-empty). For pointer `*schemas.EnvVar` fields, use `field != nil && field.GetValue() != ""` to avoid nil dereferences. This rule should be applied across all Go source files in the repo.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2509
File: framework/logstore/hybrid.go:104-117
Timestamp: 2026-04-04T14:03:25.451Z
Learning: In maximhq/bifrost, when enqueueing to a channel that may be concurrently closed, follow the established race-safe pattern used for log/upload write queues: (1) check the `atomic.Bool` closed flag before attempting the send, then (2) `defer recover()` around the send to handle a possible `send on closed channel` panic that races the flag check. Do not flag this pattern or suggest replacing it with alternatives like `sync.Once` or `ProviderQueue`. Note: the `ProviderQueue` pattern is specific to provider request queues in `core/bifrost.go` and should not be applied as a replacement for these log/upload enqueue/write queues.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2341
File: transports/bifrost-http/handlers/webrtc_realtime.go:1045-1066
Timestamp: 2026-04-07T10:37:38.913Z
Learning: In this repository’s Go code, treat governance context keys as limited to the ones that are actually propagated as request-context values: VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys, and PluginName. Do not suggest adding or flagging missing propagation of BifrostContextKeyGovernanceBusinessUnitID or BifrostContextKeyGovernanceBusinessUnitName in any Go context copy/list. These Business Unit keys are UI-only and appear exclusively in TSX, not in Go request-context propagation (including core/schemas/bifrost.go and bifrost-enterprise/).
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 2605
File: framework/vectorstore/redis.go:1670-1686
Timestamp: 2026-04-09T19:27:39.791Z
Learning: In maximhq/bifrost, the method `(*schemas.EnvVar).CoerceBool(defaultValue bool)` is nil-receiver safe: it begins with `if e == nil { return defaultValue }`, so calling it on an optional `*schemas.EnvVar` (i.e., `nil` possible) should not be treated as a potential nil-dereference panic. During review, do not flag unconditional calls to `.CoerceBool(...)` on optional/nullable `*schemas.EnvVar` fields as nil-deref issues, since the implementation guards against `e == nil` in `core/schemas/envvar.go`.
Learnt from: sammaji
Repo: maximhq/bifrost PR: 2039
File: plugins/compat/main.go:73-83
Timestamp: 2026-04-10T07:06:36.047Z
Learning: In maximhq/bifrost, treat `BifrostContextKeyChangeRequestType` set via `ctx.SetValue` (typed as `*schemas.BifrostContext` per request) as non-stale across requests. Because `core/utils.go`’s `clearCtxForFallback` clears `BifrostContextKeyChangeRequestType` during fallback retries, plugins may set this key in `PreLLMHook` without resetting it and should not be flagged for missing cleanup, as request/fallback lifecycle guarantees a clean per-request state at the start of each request (or fallback-retried) run.
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 2559
File: plugins/semanticcache/utils.go:450-459
Timestamp: 2026-04-10T07:37:36.230Z
Learning: In maximhq/bifrost’s semantic cache plugin (plugins/semanticcache), treat the semantic cache as text-only. For EmbeddingRequest handling, do not require an empty-string or non-text guard inside extractTextForEmbedding/related text-extraction codepaths for multimodal (image/audio/file-only) inputs—non-text requests are intentionally routed out of the semantic cache by the upstream routing layer. Review multimodal-only behavior in the routing layer instead of flagging the lack of a non-text guard in text extraction within this plugin.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 2893
File: transports/config.schema.json:1548-1550
Timestamp: 2026-04-21T12:58:33.892Z
Learning: In the maximhq/bifrost public repository, `access_profiles` / `AccessProfiles` is an enterprise-only feature implemented in the private enterprise codebase. During code review of the public repo, do not flag issues like “missing Go struct fields” (e.g., in `ConfigData`, `GovernanceConfig`, or related types) or related unmarshaling/handling gaps specifically for `access_profiles`, since the corresponding fields and runtime behavior are not present in the public code.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 2935
File: transports/bifrost-http/integrations/pydanticai.go:49-54
Timestamp: 2026-04-22T13:14:01.847Z
Learning: When reviewing Go code in maximhq/bifrost, do not flag `resp.WithDefaults()` as a potential nil-pointer panic if `resp` is a `*schemas.BifrostResponsesResponse`. The method `(*schemas.BifrostResponsesResponse).WithDefaults()` (in `core/schemas/responses.go`) is nil-receiver safe: it immediately returns `nil` when `resp == nil`, so calls do not panic even without a prior nil check.
Learnt from: roroghost17
Repo: maximhq/bifrost PR: 2937
File: core/providers/anthropic/request_builder.go:1-1
Timestamp: 2026-04-23T11:26:47.834Z
Learning: In maximhq/bifrost, underscores in non-test Go filenames are an established naming convention (e.g., `count_tokens.go`, `large_payload.go`, `request_builder.go`). During code review, do not flag underscore-containing Go filenames as a naming violation or suggest renaming them. This exception applies only to non-test `.go` files; the general rule may still apply to test files if a separate convention exists.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3079
File: plugins/semanticcache/utils.go:77-77
Timestamp: 2026-04-27T11:53:53.785Z
Learning: In this repo’s semanticcache plugin, `generateEmbedding` may be called from `generateEmbeddingsForStorage` and `performSemanticSearch` only when `plugin.embeddingRequestExecutor != nil` is checked in `PreLLMHook` (per the existing invariants). When reviewing code in `plugins/semanticcache`, do not require an additional nil-guard for `embeddingRequestExecutor` inside `generateEmbedding` itself if all call paths are already protected by the `!= nil` checks at the call sites.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3166
File: framework/configstore/clientconfig.go:318-332
Timestamp: 2026-04-30T13:35:05.360Z
Learning: In maximhq/bifrost, treat `(*schemas.EnvVar).IsSet()` as nil-receiver safe (it returns `false` when the receiver is `nil`, implemented in `core/schemas/envvar.go`). During code review, do NOT flag unconditional calls like `envVar.IsSet()` as potential nil-dereferences when `envVar` is an optional/nullable `*schemas.EnvVar`. Similarly, follow-on accesses are safe when guarded: calls like `x.IsFromEnv()` / `x.GetValue()` or field access inside `if x.IsSet() { ... }` are safe because `IsSet()` returning `true` implies `x` is non-nil.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3166
File: transports/bifrost-http/lib/config.go:3265-3276
Timestamp: 2026-04-30T13:35:17.403Z
Learning: In maximhq/bifrost, `(*schemas.EnvVar).GetValue()` implemented in `core/schemas/envvar.go` is nil-receiver safe (it immediately checks `if e == nil { return "" }`). During code reviews, you can assume calling `.GetValue()` on an optional `*schemas.EnvVar` pointer will not panic, and you should not recommend adding explicit nil guards solely to protect `.GetValue()` calls.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 3230
File: transports/bifrost-http/handlers/oauth2.go:213-224
Timestamp: 2026-05-05T14:00:01.435Z
Learning: In maximhq/bifrost, `schemas.EnvVar` implements `driver.Valuer` via `core/schemas/envvar.go`: `EnvVar.Value()` is guaranteed to return `(driver.Value, error)` where `error` is always `nil` and the `driver.Value` is always a `string` (sourced from either `EnvVar` or `Val`, both `string` fields). Therefore, during Go reviews, do not flag (a) silent error ignores using `_` for the `error` result of `EnvVar.Value()` and do not treat (b) type assertions `v.(string)` on the `EnvVar.Value()` result as unsafe—these operations should be structurally guaranteed to succeed.
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 2559
File: core/schemas/embedding.go:226-232
Timestamp: 2026-05-07T12:46:19.669Z
Learning: In maximhq/bifrost, the live embedding schema no longer contains `EmbeddingStruct` and therefore `EmbeddingArray` / `Embedding2DArray` fields do not exist anywhere. The active representation is `EmbeddingsByType` in `core/schemas/embedding.go` with `Float []float64`, `Int8 []int8`, `Uint8 []uint8`, `Binary []int8`, `Ubinary []uint8`, and `Base64 *string`. During code reviews, do not flag, require handling, or suggest supporting `EmbeddingArray`/`Embedding2DArray` (or `EmbeddingStruct`) anywhere in the codebase. Also do not suggest changing `Binary` to `[]byte` (an alias for `[]uint8`) because Cohere’s binary embeddings include negative signed integers; `Ubinary` should remain `[]uint8` because Cohere’s ubinary embeddings are 0–255 unsigned values.
2ee69fd to
59459da
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugins/semanticcache/search.go (1)
344-355:⚠️ Potential issue | 🟠 MajorStill fail closed on malformed cached stream chunks.
Skipping a bad chunk turns a corrupt cache entry into a partial replay. If the malformed chunk is the last one,
stampCacheDebugForHitnever runs, so downstream final-chunk handling/log finalization can be skipped entirely. Abort replay on the first decode/validation failure and treat the entry as a cache miss instead.Also applies to: 364-377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/search.go` around lines 344 - 355, The current goroutine that iterates streamArray and unmarshals into schemas.BifrostResponse (inside the anonymous func that closes streamChan) silently skips malformed chunks by logging via plugin.logger.Warn and continuing; change this to fail closed: on the first json.Unmarshal or validation error for a chunk (in the loop over streamArray) immediately log the error, abort the replay by closing streamChan and returning (so no further chunks are sent), and ensure the cache is treated as a miss by invoking the same failure path used elsewhere (so stampCacheDebugForHit is not called); apply the same behavior to the later block handling indices 364-377 to keep consistent fail-closed semantics.
🧹 Nitpick comments (3)
ui/app/workspace/config/views/pluginsForm.tsx (2)
98-98: 💤 Low valueFilename uses camelCase instead of PascalCase.
Per coding guidelines, React component files should use PascalCase for filenames (
PluginsForm.tsxinstead ofpluginsForm.tsx). Since this is an existing file, this can be addressed in a separate housekeeping refactor.🤖 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/config/views/pluginsForm.tsx` at line 98, The file name uses camelCase but React component files should be PascalCase; rename the file to PluginsForm.tsx and update all imports/usages that reference the old filename (e.g., any import of PluginsForm from "pluginsForm" or similar) to the new PascalCase module name so the exported component PluginsForm continues to be imported correctly throughout the codebase; ensure build/test pass after the rename.
315-351: 💤 Low valueConsider adding
data-testidattributes for E2E test compatibility.The new provider
SelectandModelMultiselectcomponents lackdata-testidattributes. Per coding guidelines, new interactive elements should include them (e.g.,data-testid="semantic-cache-provider-select",data-testid="semantic-cache-embedding-model-select").Based on learnings, this can be deferred to a dedicated PR that also updates related E2E tests.
🤖 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/config/views/pluginsForm.tsx` around lines 315 - 351, The Select for choosing provider (rendered with the Select/SelectTrigger/SelectContent/SelectItem components) and the ModelMultiselect (props: inputId="embedding_model", isSingleSelect, provider, value, onChange) are missing data-testid attributes required for E2E tests; add data-testid="semantic-cache-provider-select" to the provider Select trigger/component and data-testid="semantic-cache-embedding-model-select" to the ModelMultiselect (or its input wrapper) while preserving existing props and behavior (updateCacheConfigLocal and cacheConfig usage remain unchanged).plugins/semanticcache/plugin_core_test.go (1)
454-480: ⚡ Quick win
custom_ttlstill doesn't exercise TTL behavior.This branch only re-checks the generic direct-hit path, so a regression in
resolveTTL/expires_athandling would still pass. Use a short TTL and assert the entry expires, or inspect the storedexpires_atmetadata directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/plugin_core_test.go` around lines 454 - 480, The test's custom_ttl branch only re-checks a direct cache hit and doesn't validate TTL expiry; update the test for the "custom_ttl" tt case to actually exercise resolveTTL/expires_at by configuring a short TTL (e.g., 1s) for that plugin, then either: (A) after the first successful cached response (response2 from setup.Client.ChatCompletionRequest), wait slightly longer than the short TTL and issue a new ChatCompletionRequest and assert a cache miss (i.e., AssertCacheMiss or that CacheDebug.CacheHit is false), or (B) directly read the stored cache metadata for that request (inspect response2.ExtraFields.CacheDebug or the underlying cache entry) and assert the expires_at value equals now + configured TTL (and that it is within an acceptable delta). Modify the test helper or fixture that builds the "custom_ttl" config and add the sleep+re-request or metadata assertion to ensure resolveTTL/expires_at behavior is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/semanticcache/main.go`:
- Around line 354-358: createCacheState is persisting cache state too early
(called before isConversationHistoryThresholdExceeded and before the ParamsHash
empty-path in PostLLMHook), causing stale cacheStates to accumulate for requests
that are never cacheable; change the flow so you do not persist the cacheState
up-front—either create a non-persisted temporary state or delay calling
plugin.createCacheState until after
plugin.isConversationHistoryThresholdExceeded returns false and after you verify
ParamsHash != "" (i.e., only persist the state when the request is actually
cacheable in PostLLMHook and related paths referenced around
createCacheState/isConversationHistoryThresholdExceeded).
- Around line 625-640: In stampCacheDebugForMiss, ensure all hit-only fields are
cleared when stamping a miss: set CacheHit=false and CacheID as already done,
and explicitly nil-out or reset HitType, Threshold, Similarity, CacheHitLatency,
ProviderUsed, ModelUsed and InputTokens on extraFields.CacheDebug so no previous
hit data leaks into miss responses; update the function (stampCacheDebugForMiss)
to clear those fields on cd (the local CacheDebug variable) before returning.
---
Duplicate comments:
In `@plugins/semanticcache/search.go`:
- Around line 344-355: The current goroutine that iterates streamArray and
unmarshals into schemas.BifrostResponse (inside the anonymous func that closes
streamChan) silently skips malformed chunks by logging via plugin.logger.Warn
and continuing; change this to fail closed: on the first json.Unmarshal or
validation error for a chunk (in the loop over streamArray) immediately log the
error, abort the replay by closing streamChan and returning (so no further
chunks are sent), and ensure the cache is treated as a miss by invoking the same
failure path used elsewhere (so stampCacheDebugForHit is not called); apply the
same behavior to the later block handling indices 364-377 to keep consistent
fail-closed semantics.
---
Nitpick comments:
In `@plugins/semanticcache/plugin_core_test.go`:
- Around line 454-480: The test's custom_ttl branch only re-checks a direct
cache hit and doesn't validate TTL expiry; update the test for the "custom_ttl"
tt case to actually exercise resolveTTL/expires_at by configuring a short TTL
(e.g., 1s) for that plugin, then either: (A) after the first successful cached
response (response2 from setup.Client.ChatCompletionRequest), wait slightly
longer than the short TTL and issue a new ChatCompletionRequest and assert a
cache miss (i.e., AssertCacheMiss or that CacheDebug.CacheHit is false), or (B)
directly read the stored cache metadata for that request (inspect
response2.ExtraFields.CacheDebug or the underlying cache entry) and assert the
expires_at value equals now + configured TTL (and that it is within an
acceptable delta). Modify the test helper or fixture that builds the
"custom_ttl" config and add the sleep+re-request or metadata assertion to ensure
resolveTTL/expires_at behavior is verified.
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Line 98: The file name uses camelCase but React component files should be
PascalCase; rename the file to PluginsForm.tsx and update all imports/usages
that reference the old filename (e.g., any import of PluginsForm from
"pluginsForm" or similar) to the new PascalCase module name so the exported
component PluginsForm continues to be imported correctly throughout the
codebase; ensure build/test pass after the rename.
- Around line 315-351: The Select for choosing provider (rendered with the
Select/SelectTrigger/SelectContent/SelectItem components) and the
ModelMultiselect (props: inputId="embedding_model", isSingleSelect, provider,
value, onChange) are missing data-testid attributes required for E2E tests; add
data-testid="semantic-cache-provider-select" to the provider Select
trigger/component and data-testid="semantic-cache-embedding-model-select" to the
ModelMultiselect (or its input wrapper) while preserving existing props and
behavior (updateCacheConfigLocal and cacheConfig usage remain 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 849d029e-ae40-4d26-b642-e346b85f6365
📒 Files selected for processing (55)
.claude/skills/docs-writer/SKILL.md.gitignorecore/schemas/bifrost.gocore/schemas/context.gocore/schemas/context_test.godocs/features/semantic-caching.mdxdocs/migration-guides/v1.5.0.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/cache.yamlframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goframework/modelcatalog/sync.goframework/vectorstore/weaviate.goplugins/logging/main.goplugins/logging/operations.goplugins/semanticcache/main.goplugins/semanticcache/main_test.goplugins/semanticcache/plugin_api_test.goplugins/semanticcache/plugin_cache_type_test.goplugins/semanticcache/plugin_conversation_config_test.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_cross_cache_test.goplugins/semanticcache/plugin_default_cache_key_test.goplugins/semanticcache/plugin_edge_cases_test.goplugins/semanticcache/plugin_embedding_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/plugin_nil_content_test.goplugins/semanticcache/plugin_no_mutation_test.goplugins/semanticcache/plugin_no_store_test.goplugins/semanticcache/plugin_normalization_test.goplugins/semanticcache/plugin_paths_test.goplugins/semanticcache/plugin_responses_test.goplugins/semanticcache/plugin_streaming_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/search.goplugins/semanticcache/state.goplugins/semanticcache/stream.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/cache_test.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/pluginsForm.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/components/filters/logsFilterSidebar.tsxui/lib/constants/logs.tsui/lib/store/apis/logsApi.tsui/lib/types/config.tsui/lib/types/logs.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- plugins/logging/operations.go
✅ Files skipped from review due to trivial changes (13)
- .gitignore
- plugins/logging/main.go
- ui/lib/constants/logs.ts
- transports/bifrost-http/handlers/logging.go
- .claude/skills/docs-writer/SKILL.md
- framework/modelcatalog/sync.go
- framework/logstore/rdb.go
- ui/lib/types/logs.ts
- ui/app/workspace/logs/sheets/logDetailView.tsx
- ui/app/workspace/logs/page.tsx
- ui/lib/types/schemas.ts
- plugins/semanticcache/stream.go
- plugins/semanticcache/plugin_no_store_test.go
🚧 Files skipped from review as they are similar to previous changes (25)
- ui/lib/store/apis/logsApi.ts
- docs/openapi/paths/management/cache.yaml
- docs/migration-guides/v1.5.0.mdx
- docs/openapi/openapi.json
- transports/bifrost-http/handlers/middlewares.go
- ui/lib/types/config.ts
- core/schemas/context.go
- plugins/semanticcache/plugin_streaming_test.go
- plugins/semanticcache/state.go
- plugins/semanticcache/plugin_image_generation_test.go
- plugins/semanticcache/plugin_vectorstore_test.go
- plugins/semanticcache/plugin_paths_test.go
- docs/features/semantic-caching.mdx
- ui/components/filters/logsFilterSidebar.tsx
- plugins/semanticcache/plugin_api_test.go
- plugins/semanticcache/plugin_normalization_test.go
- framework/logstore/matviews.go
- docs/openapi/openapi.yaml
- plugins/semanticcache/plugin_embedding_test.go
- plugins/semanticcache/plugin_default_cache_key_test.go
- plugins/semanticcache/utils.go
- plugins/semanticcache/plugin_nil_content_test.go
- transports/bifrost-http/handlers/cache.go
- plugins/semanticcache/test_utils.go
- transports/bifrost-http/handlers/cache_test.go
Comment on lines
+625
to
+640
| func (plugin *Plugin) stampCacheDebugForMiss(state *cacheState, extraFields *schemas.BifrostResponseExtraFields, storageID string, isStream, isFinalChunk bool) { | ||
| if isStream && !isFinalChunk { | ||
| return | ||
| } | ||
| if extraFields.CacheDebug == nil { | ||
| extraFields.CacheDebug = &schemas.BifrostCacheDebug{} | ||
| } | ||
| cd := extraFields.CacheDebug | ||
| cd.CacheHit = false | ||
| cd.CacheID = bifrost.Ptr(storageID) | ||
| if state.EmbeddingsInputTokens > 0 { | ||
| inputTokens := state.EmbeddingsInputTokens | ||
| cd.ProviderUsed = bifrost.Ptr(string(plugin.config.Provider)) | ||
| cd.ModelUsed = bifrost.Ptr(plugin.config.EmbeddingModel) | ||
| cd.InputTokens = &inputTokens | ||
| } |
Contributor
There was a problem hiding this comment.
Clear hit-only cache_debug fields when stamping misses.
This path only flips CacheHit=false and sets CacheID. If extraFields.CacheDebug is already populated, HitType, Threshold, Similarity, CacheHitLatency, and the requested/provider/model fields leak through, which makes the new cache_hit_types filtering and UI badges internally inconsistent on misses.
Suggested fix
cd := extraFields.CacheDebug
cd.CacheHit = false
cd.CacheID = bifrost.Ptr(storageID)
+ cd.HitType = nil
+ cd.RequestedProvider = nil
+ cd.RequestedModel = nil
+ cd.CacheHitLatency = nil
+ cd.Threshold = nil
+ cd.Similarity = nil
+ cd.ProviderUsed = nil
+ cd.ModelUsed = nil
+ cd.InputTokens = nil
if state.EmbeddingsInputTokens > 0 {
inputTokens := state.EmbeddingsInputTokens
cd.ProviderUsed = bifrost.Ptr(string(plugin.config.Provider))
cd.ModelUsed = bifrost.Ptr(plugin.config.EmbeddingModel)
cd.InputTokens = &inputTokens
}📝 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
| func (plugin *Plugin) stampCacheDebugForMiss(state *cacheState, extraFields *schemas.BifrostResponseExtraFields, storageID string, isStream, isFinalChunk bool) { | |
| if isStream && !isFinalChunk { | |
| return | |
| } | |
| if extraFields.CacheDebug == nil { | |
| extraFields.CacheDebug = &schemas.BifrostCacheDebug{} | |
| } | |
| cd := extraFields.CacheDebug | |
| cd.CacheHit = false | |
| cd.CacheID = bifrost.Ptr(storageID) | |
| if state.EmbeddingsInputTokens > 0 { | |
| inputTokens := state.EmbeddingsInputTokens | |
| cd.ProviderUsed = bifrost.Ptr(string(plugin.config.Provider)) | |
| cd.ModelUsed = bifrost.Ptr(plugin.config.EmbeddingModel) | |
| cd.InputTokens = &inputTokens | |
| } | |
| func (plugin *Plugin) stampCacheDebugForMiss(state *cacheState, extraFields *schemas.BifrostResponseExtraFields, storageID string, isStream, isFinalChunk bool) { | |
| if isStream && !isFinalChunk { | |
| return | |
| } | |
| if extraFields.CacheDebug == nil { | |
| extraFields.CacheDebug = &schemas.BifrostCacheDebug{} | |
| } | |
| cd := extraFields.CacheDebug | |
| cd.CacheHit = false | |
| cd.CacheID = bifrost.Ptr(storageID) | |
| cd.HitType = nil | |
| cd.RequestedProvider = nil | |
| cd.RequestedModel = nil | |
| cd.CacheHitLatency = nil | |
| cd.Threshold = nil | |
| cd.Similarity = nil | |
| cd.ProviderUsed = nil | |
| cd.ModelUsed = nil | |
| cd.InputTokens = nil | |
| if state.EmbeddingsInputTokens > 0 { | |
| inputTokens := state.EmbeddingsInputTokens | |
| cd.ProviderUsed = bifrost.Ptr(string(plugin.config.Provider)) | |
| cd.ModelUsed = bifrost.Ptr(plugin.config.EmbeddingModel) | |
| cd.InputTokens = &inputTokens | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/semanticcache/main.go` around lines 625 - 640, In
stampCacheDebugForMiss, ensure all hit-only fields are cleared when stamping a
miss: set CacheHit=false and CacheID as already done, and explicitly nil-out or
reset HitType, Threshold, Similarity, CacheHitLatency, ProviderUsed, ModelUsed
and InputTokens on extraFields.CacheDebug so no previous hit data leaks into
miss responses; update the function (stampCacheDebugForMiss) to clear those
fields on cd (the local CacheDebug variable) before returning.
Collaborator
Author
There was a problem hiding this comment.
Looked into this and there's no real path that triggers stale CacheDebug fields here. The response object reaching PostLLMHook is fresh from the provider; providers don't set CacheDebug, only this plugin does. We stamp either hit (in PreLLMHook short-circuit, on the cached response) or miss (in PostLLMHook, on the fresh response) — never both on the same object. For streams, each chunk is a separate BifrostResponse so there's no carry-over either. Closing this out — happy to revisit if a concrete pipeline that triggers it comes up.
Contributor
There was a problem hiding this comment.
@Pratham-Mishra04, that's a correct and thorough analysis — thank you for tracing the execution paths. The two key guardrails confirm your point:
shouldSkipCachingreturns early whencacheDebug.CacheHit == true, sostampCacheDebugForMissis never reached for hit-replay responses.- Fresh provider responses arrive without any
CacheDebugpopulated (providers have no awareness of this plugin's fields), so there is nothing stale to clear.
The scenario I was guarding against doesn't occur in this pipeline. Happy to revisit if a future plugin or retry path introduces shared response objects, but for now the concern doesn't apply. I'll withdraw it.
🧠 Learnings used
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1312
File: framework/modelcatalog/pricing.go:276-426
Timestamp: 2026-01-14T04:40:11.480Z
Learning: In the Bifrost codebase, ImageUsage and other usage types guarantee that TotalTokens is populated (computed as InputTokens + OutputTokens if providers don’t supply TotalTokens). Reviewers can rely on this invariant and should not assume TotalTokens may be missing when input/output tokens exist. When implementing tiering logic or token-based decisions, you can safely use TotalTokens without extra null/zero guards, provided you’re in a context where InputTokens and OutputTokens are present. If a branch might discard tokens, ensure the invariant is preserved or add explicit checks only where the inputs are confirmed to be valid.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1326
File: plugins/semanticcache/utils.go:11-11
Timestamp: 2026-01-14T10:34:23.134Z
Learning: In files under plugins/semanticcache (e.g., utils.go and similar), when req.RequestType is ImageGenerationRequest or ImageGenerationStreamRequest, the corresponding typed field (req.ImageGenerationRequest) is guaranteed non-nil by the framework. Do not insert nil checks for these fields in these cases. This pattern should apply to all similar files in that directory; for other request types, keep appropriate nil checks as needed. Consider adding unit tests to verify non-nil guarantees and refactor accordingly.
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1326
File: plugins/semanticcache/test_utils.go:545-559
Timestamp: 2026-01-14T13:30:28.760Z
Learning: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability. Replace occurrences of &value where a *T is expected with bifrost.Ptr(value) (or an equivalent call) and ensure the function is in scope and used correctly for the target pointer type.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1352
File: plugins/litellmcompat/main.go:76-89
Timestamp: 2026-01-18T07:00:26.487Z
Learning: In the Bifrost plugin system, disabled plugins are not added to the plugin chain; they are loaded/unloaded via ReloadPlugin/RemovePlugin based on configuration. Therefore, internal checks for an Enabled flag inside PreHook or PostHook are unnecessary, as these methods will not be invoked for disabled plugins. Reviewers should ensure code relies on the loader to exclude disabled plugins and consider removing redundant Enabled checks in plugin methods. This guideline applies to all Go files under the plugins directory.
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 1740
File: transports/bifrost-http/handlers/governance.go:3168-3214
Timestamp: 2026-02-23T07:58:44.087Z
Learning: In this codebase using GORM, models with CreatedAt and UpdatedAt fields of type time.Time tagged with gorm:"autoCreateTime" and gorm:"autoUpdateTime" are populated automatically by GORM on insert/update. Do not manually set them with time.Now(). Remove any manual initialization; rely on GORM's automatic timestamps. This applies to all Go files with GORM models in the repository.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1670
File: core/providers/anthropic/anthropic.go:689-707
Timestamp: 2026-02-24T04:21:32.824Z
Learning: In Go streaming handlers that reuse pooled response objects (e.g., BifrostChatResponse, BifrostResponsesResponse.Response) via ProcessAndSendResponse, do not release them back to their pools while asynchronous readers (PostLLMHook goroutines) may still access them. Releasing between Acquire and use can cause data races and panics when fields are read by the goroutines. Rely on GC after all references are dropped, and apply this safety pattern to all pooled response types passed through ProcessAndSendResponse in streaming contexts. This should be documented and enforced consistently across all relevant Go files.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1836
File: core/providers/utils/utils.go:1994-2019
Timestamp: 2026-03-01T13:11:33.245Z
Learning: Enforce the repository-wide convention: all object pools must use raw sync.Pool (not pool.New[T]() or generic pool builders). When reviewing any Go files, verify that pooling code uses sync.Pool directly and constistent with the examples in maximhq/bifrost (e.g., core/bifrost.go, core/providers/anthropic/anthropic.go, core/providers/cohere/cohere.go, core/schemas/plugin.go, framework/tracing/store.go). In particular, do not introduce pool.New[T]() usage; ensure existing pool implementations remain the raw sync.Pool pattern.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2152
File: framework/logstore/tables.go:422-431
Timestamp: 2026-03-18T09:04:27.884Z
Learning: Do not flag usages of new(expr) in Go code as compile-time errors. Starting with Go 1.26, new() accepts an expression operand (e.g., new(string(data))), and is valid syntax. Reviewers should only flag actual invalid uses per the Go version used in CI, and assume new(expr) forms are allowed across Go files.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2322
File: core/providers/anthropic/responses.go:1988-1992
Timestamp: 2026-03-27T09:20:29.538Z
Learning: In maximhq/bifrost, `BifrostContext` (`core/schemas/context.go`) is a mutable shared context. Its `(*BifrostContext).SetValue(key, value any)` is a pointer-receiver that mutates the internal `userValues map[any]any` in place under a write mutex; it does not create a derived context like Go’s `context.WithValue`. Therefore, when reviewing code, do not flag `SetValue` usage as failing to “propagate” context—subsequent `ctx.Value()` reads on the same `*BifrostContext` pointer should see the updated value immediately.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2339
File: plugins/logging/utils.go:531-543
Timestamp: 2026-03-31T05:42:40.984Z
Learning: When reviewing Go code that uses `schemas.ResponsesMessageContent` (as in `plugins/logging/utils.go`), treat `ContentStr` and `ContentBlocks` as mutually exclusive content sources. The type’s `MarshalJSON` enforces that: if `ContentStr != nil`, it is the sole content source and code should not include or suggest a fallback-to-`ContentBlocks` guard when `ContentStr` is non-nil (even if it might be empty). Conversely, only use `ContentBlocks` when `ContentStr` is nil, per the schema contract.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2339
File: plugins/logging/main.go:740-744
Timestamp: 2026-04-03T10:46:22.677Z
Learning: In maximhq/bifrost, Bifrost context key/value assignments are done with `(*BifrostContext).SetValue(key, value)`, not with `context.WithValue`. During code review/searches for where `BifrostContextKey*` constants are set, look for `.SetValue(` patterns (e.g., `rg -n 'SetValue.*BifrostContextKey'`) rather than `context.WithValue`/`WithValue.*Key` to avoid false findings that a key is never set.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2510
File: framework/objectstore/gcs.go:32-43
Timestamp: 2026-04-04T10:05:42.632Z
Learning: In maximhq/bifrost, `schemas.EnvVar` does not implement `IsDefined()`. Reviewers should not flag or suggest calling `schemas.EnvVar.IsDefined()` anywhere in the repository. To check whether an EnvVar config is both present and resolved, use `cfg.Field != nil && cfg.Field.GetValue() != ""` (where `cfg.Field` is the `*schemas.EnvVar` pointer field being evaluated).
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2509
File: framework/logstore/store.go:110-127
Timestamp: 2026-04-04T10:30:13.550Z
Learning: In maximhq/bifrost, do not reference `EnvVar.IsDefined()` (it does not exist on `schemas.EnvVar`). To validate a non-pointer `schemas.EnvVar` field, check `field.GetValue() == ""` (for “defined” it should be non-empty). For pointer `*schemas.EnvVar` fields, use `field != nil && field.GetValue() != ""` to avoid nil dereferences. This rule should be applied across all Go source files in the repo.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2509
File: framework/logstore/hybrid.go:104-117
Timestamp: 2026-04-04T14:03:25.451Z
Learning: In maximhq/bifrost, when enqueueing to a channel that may be concurrently closed, follow the established race-safe pattern used for log/upload write queues: (1) check the `atomic.Bool` closed flag before attempting the send, then (2) `defer recover()` around the send to handle a possible `send on closed channel` panic that races the flag check. Do not flag this pattern or suggest replacing it with alternatives like `sync.Once` or `ProviderQueue`. Note: the `ProviderQueue` pattern is specific to provider request queues in `core/bifrost.go` and should not be applied as a replacement for these log/upload enqueue/write queues.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2341
File: transports/bifrost-http/handlers/webrtc_realtime.go:1045-1066
Timestamp: 2026-04-07T10:37:38.913Z
Learning: In this repository’s Go code, treat governance context keys as limited to the ones that are actually propagated as request-context values: VirtualKey, Team, Customer, User, RoutingRule, IncludeOnlyKeys, and PluginName. Do not suggest adding or flagging missing propagation of BifrostContextKeyGovernanceBusinessUnitID or BifrostContextKeyGovernanceBusinessUnitName in any Go context copy/list. These Business Unit keys are UI-only and appear exclusively in TSX, not in Go request-context propagation (including core/schemas/bifrost.go and bifrost-enterprise/).
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 2605
File: framework/vectorstore/redis.go:1670-1686
Timestamp: 2026-04-09T19:27:39.791Z
Learning: In maximhq/bifrost, the method `(*schemas.EnvVar).CoerceBool(defaultValue bool)` is nil-receiver safe: it begins with `if e == nil { return defaultValue }`, so calling it on an optional `*schemas.EnvVar` (i.e., `nil` possible) should not be treated as a potential nil-dereference panic. During review, do not flag unconditional calls to `.CoerceBool(...)` on optional/nullable `*schemas.EnvVar` fields as nil-deref issues, since the implementation guards against `e == nil` in `core/schemas/envvar.go`.
Learnt from: sammaji
Repo: maximhq/bifrost PR: 2039
File: plugins/compat/main.go:73-83
Timestamp: 2026-04-10T07:06:36.047Z
Learning: In maximhq/bifrost, treat `BifrostContextKeyChangeRequestType` set via `ctx.SetValue` (typed as `*schemas.BifrostContext` per request) as non-stale across requests. Because `core/utils.go`’s `clearCtxForFallback` clears `BifrostContextKeyChangeRequestType` during fallback retries, plugins may set this key in `PreLLMHook` without resetting it and should not be flagged for missing cleanup, as request/fallback lifecycle guarantees a clean per-request state at the start of each request (or fallback-retried) run.
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 2559
File: plugins/semanticcache/utils.go:450-459
Timestamp: 2026-04-10T07:37:36.230Z
Learning: In maximhq/bifrost’s semantic cache plugin (plugins/semanticcache), treat the semantic cache as text-only. For EmbeddingRequest handling, do not require an empty-string or non-text guard inside extractTextForEmbedding/related text-extraction codepaths for multimodal (image/audio/file-only) inputs—non-text requests are intentionally routed out of the semantic cache by the upstream routing layer. Review multimodal-only behavior in the routing layer instead of flagging the lack of a non-text guard in text extraction within this plugin.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 2893
File: transports/config.schema.json:1548-1550
Timestamp: 2026-04-21T12:58:33.892Z
Learning: In the maximhq/bifrost public repository, `access_profiles` / `AccessProfiles` is an enterprise-only feature implemented in the private enterprise codebase. During code review of the public repo, do not flag issues like “missing Go struct fields” (e.g., in `ConfigData`, `GovernanceConfig`, or related types) or related unmarshaling/handling gaps specifically for `access_profiles`, since the corresponding fields and runtime behavior are not present in the public code.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 2935
File: transports/bifrost-http/integrations/pydanticai.go:49-54
Timestamp: 2026-04-22T13:14:01.847Z
Learning: When reviewing Go code in maximhq/bifrost, do not flag `resp.WithDefaults()` as a potential nil-pointer panic if `resp` is a `*schemas.BifrostResponsesResponse`. The method `(*schemas.BifrostResponsesResponse).WithDefaults()` (in `core/schemas/responses.go`) is nil-receiver safe: it immediately returns `nil` when `resp == nil`, so calls do not panic even without a prior nil check.
Learnt from: roroghost17
Repo: maximhq/bifrost PR: 2937
File: core/providers/anthropic/request_builder.go:1-1
Timestamp: 2026-04-23T11:26:47.834Z
Learning: In maximhq/bifrost, underscores in non-test Go filenames are an established naming convention (e.g., `count_tokens.go`, `large_payload.go`, `request_builder.go`). During code review, do not flag underscore-containing Go filenames as a naming violation or suggest renaming them. This exception applies only to non-test `.go` files; the general rule may still apply to test files if a separate convention exists.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3079
File: plugins/semanticcache/utils.go:77-77
Timestamp: 2026-04-27T11:53:53.785Z
Learning: In this repo’s semanticcache plugin, `generateEmbedding` may be called from `generateEmbeddingsForStorage` and `performSemanticSearch` only when `plugin.embeddingRequestExecutor != nil` is checked in `PreLLMHook` (per the existing invariants). When reviewing code in `plugins/semanticcache`, do not require an additional nil-guard for `embeddingRequestExecutor` inside `generateEmbedding` itself if all call paths are already protected by the `!= nil` checks at the call sites.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3166
File: framework/configstore/clientconfig.go:318-332
Timestamp: 2026-04-30T13:35:05.360Z
Learning: In maximhq/bifrost, treat `(*schemas.EnvVar).IsSet()` as nil-receiver safe (it returns `false` when the receiver is `nil`, implemented in `core/schemas/envvar.go`). During code review, do NOT flag unconditional calls like `envVar.IsSet()` as potential nil-dereferences when `envVar` is an optional/nullable `*schemas.EnvVar`. Similarly, follow-on accesses are safe when guarded: calls like `x.IsFromEnv()` / `x.GetValue()` or field access inside `if x.IsSet() { ... }` are safe because `IsSet()` returning `true` implies `x` is non-nil.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 3166
File: transports/bifrost-http/lib/config.go:3265-3276
Timestamp: 2026-04-30T13:35:17.403Z
Learning: In maximhq/bifrost, `(*schemas.EnvVar).GetValue()` implemented in `core/schemas/envvar.go` is nil-receiver safe (it immediately checks `if e == nil { return "" }`). During code reviews, you can assume calling `.GetValue()` on an optional `*schemas.EnvVar` pointer will not panic, and you should not recommend adding explicit nil guards solely to protect `.GetValue()` calls.
Learnt from: BearTS
Repo: maximhq/bifrost PR: 3230
File: transports/bifrost-http/handlers/oauth2.go:213-224
Timestamp: 2026-05-05T14:00:01.435Z
Learning: In maximhq/bifrost, `schemas.EnvVar` implements `driver.Valuer` via `core/schemas/envvar.go`: `EnvVar.Value()` is guaranteed to return `(driver.Value, error)` where `error` is always `nil` and the `driver.Value` is always a `string` (sourced from either `EnvVar` or `Val`, both `string` fields). Therefore, during Go reviews, do not flag (a) silent error ignores using `_` for the `error` result of `EnvVar.Value()` and do not treat (b) type assertions `v.(string)` on the `EnvVar.Value()` result as unsafe—these operations should be structurally guaranteed to succeed.
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 2559
File: core/schemas/embedding.go:226-232
Timestamp: 2026-05-07T12:46:19.669Z
Learning: In maximhq/bifrost, the live embedding schema no longer contains `EmbeddingStruct` and therefore `EmbeddingArray` / `Embedding2DArray` fields do not exist anywhere. The active representation is `EmbeddingsByType` in `core/schemas/embedding.go` with `Float []float64`, `Int8 []int8`, `Uint8 []uint8`, `Binary []int8`, `Ubinary []uint8`, and `Base64 *string`. During code reviews, do not flag, require handling, or suggest supporting `EmbeddingArray`/`Embedding2DArray` (or `EmbeddingStruct`) anywhere in the codebase. Also do not suggest changing `Binary` to `[]byte` (an alias for `[]uint8`) because Cohere’s binary embeddings include negative signed integers; `Ubinary` should remain `[]uint8` because Cohere’s ubinary embeddings are 0–255 unsigned values.
59459da to
d03a719
Compare
This was referenced May 5, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
plugins/semanticcache/main.go (2)
354-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefer
createCacheStateuntil the request is confirmed cacheable.
createCacheState(requestID)is called at Line 355 beforeisConversationHistoryThresholdExceeded(Line 357). When the threshold check returns early, the state is created but never cleared on the normal request path — it only gets reaped by the background cleanup loop. Under high-volume traffic with many uncached requests, this causes avoidablecacheStatesgrowth.Consider moving state creation after the threshold check, or lazily creating it only when caching will proceed.
🛠️ Proposed fix sketch
func (plugin *Plugin) PreLLMHook(ctx *schemas.BifrostContext, req *schemas.BifrostRequest) (*schemas.BifrostRequest, *schemas.LLMPluginShortCircuit, error) { // ... cache key and request ID checks ... if !isSemanticCacheSupportedRequestType(req.RequestType) { return req, nil, nil } - // Create state up front so a reused/retried request ID never inherits stale fields. - state := plugin.createCacheState(requestID) - - if plugin.isConversationHistoryThresholdExceeded(state, req) { + if plugin.isConversationHistoryThresholdExceeded(req) { return req, nil, nil } + // Create state only after confirming the request is cacheable. + state := plugin.createCacheState(requestID) + performDirectSearch, performSemanticSearch := plugin.resolveCacheTypes(ctx) // ... rest of the method ...This requires updating
isConversationHistoryThresholdExceededto not depend onstate, or passing a temporary/stack-local struct for the check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/main.go` around lines 354 - 358, createCacheState(requestID) is being called before the cacheability check, causing unused entries to accumulate in cacheStates when isConversationHistoryThresholdExceeded(req) returns early; move or defer creation of state by calling createCacheState only after isConversationHistoryThresholdExceeded returns false (i.e., after the request is confirmed cacheable), or change isConversationHistoryThresholdExceeded to accept no state (or a temporary stack-local struct) so the threshold check can run without mutating/allocating cache state; update call sites to createCacheState(requestID) only when you will proceed to caching and ensure any code paths that relied on state are adjusted accordingly.
625-641:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear hit-only fields when stamping cache misses.
stampCacheDebugForMisssetsCacheHit=falseandCacheID, but ifextraFields.CacheDebugwas previously populated (e.g., from an earlier hook or partial state), hit-only fields likeHitType,Threshold,Similarity,CacheHitLatency,RequestedProvider, andRequestedModelmay leak through. This can cause inconsistentcache_hit_typesfiltering and UI badges.🛠️ Proposed fix
func (plugin *Plugin) stampCacheDebugForMiss(state *cacheState, extraFields *schemas.BifrostResponseExtraFields, storageID string, isStream, isFinalChunk bool) { if isStream && !isFinalChunk { return } if extraFields.CacheDebug == nil { extraFields.CacheDebug = &schemas.BifrostCacheDebug{} } cd := extraFields.CacheDebug cd.CacheHit = false cd.CacheID = bifrost.Ptr(storageID) + // Clear hit-only fields to prevent stale data from leaking on misses + cd.HitType = nil + cd.Threshold = nil + cd.Similarity = nil + cd.CacheHitLatency = nil + cd.RequestedProvider = nil + cd.RequestedModel = nil if state.EmbeddingsInputTokens > 0 { inputTokens := state.EmbeddingsInputTokens cd.ProviderUsed = bifrost.Ptr(string(plugin.config.Provider)) cd.ModelUsed = bifrost.Ptr(plugin.config.EmbeddingModel) cd.InputTokens = &inputTokens + } else { + cd.ProviderUsed = nil + cd.ModelUsed = nil + cd.InputTokens = nil } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/main.go` around lines 625 - 641, stampCacheDebugForMiss must clear any hit-only fields on extraFields.CacheDebug to avoid leaking prior hit data; update the function (stampCacheDebugForMiss) to explicitly zero or nil out HitType, Threshold, Similarity, CacheHitLatency, RequestedProvider, and RequestedModel on cd before setting CacheHit=false and CacheID, leaving the existing embeddings-related assignments intact so only those hit-specific fields are cleared for a miss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@plugins/semanticcache/main.go`:
- Around line 354-358: createCacheState(requestID) is being called before the
cacheability check, causing unused entries to accumulate in cacheStates when
isConversationHistoryThresholdExceeded(req) returns early; move or defer
creation of state by calling createCacheState only after
isConversationHistoryThresholdExceeded returns false (i.e., after the request is
confirmed cacheable), or change isConversationHistoryThresholdExceeded to accept
no state (or a temporary stack-local struct) so the threshold check can run
without mutating/allocating cache state; update call sites to
createCacheState(requestID) only when you will proceed to caching and ensure any
code paths that relied on state are adjusted accordingly.
- Around line 625-641: stampCacheDebugForMiss must clear any hit-only fields on
extraFields.CacheDebug to avoid leaking prior hit data; update the function
(stampCacheDebugForMiss) to explicitly zero or nil out HitType, Threshold,
Similarity, CacheHitLatency, RequestedProvider, and RequestedModel on cd before
setting CacheHit=false and CacheID, leaving the existing embeddings-related
assignments intact so only those hit-specific fields are cleared for a miss.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af4ddbe1-9b1e-4d53-b76a-4f26cb9584fc
📒 Files selected for processing (55)
.claude/skills/docs-writer/SKILL.md.gitignorecore/schemas/bifrost.gocore/schemas/context.gocore/schemas/context_test.godocs/features/semantic-caching.mdxdocs/migration-guides/v1.5.0.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/cache.yamlframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goframework/modelcatalog/sync.goframework/vectorstore/weaviate.goplugins/logging/main.goplugins/logging/operations.goplugins/semanticcache/main.goplugins/semanticcache/main_test.goplugins/semanticcache/plugin_api_test.goplugins/semanticcache/plugin_cache_type_test.goplugins/semanticcache/plugin_conversation_config_test.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_cross_cache_test.goplugins/semanticcache/plugin_default_cache_key_test.goplugins/semanticcache/plugin_edge_cases_test.goplugins/semanticcache/plugin_embedding_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/plugin_nil_content_test.goplugins/semanticcache/plugin_no_mutation_test.goplugins/semanticcache/plugin_no_store_test.goplugins/semanticcache/plugin_normalization_test.goplugins/semanticcache/plugin_paths_test.goplugins/semanticcache/plugin_responses_test.goplugins/semanticcache/plugin_streaming_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/search.goplugins/semanticcache/state.goplugins/semanticcache/stream.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/cache_test.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/pluginsForm.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/components/filters/logsFilterSidebar.tsxui/lib/constants/logs.tsui/lib/store/apis/logsApi.tsui/lib/types/config.tsui/lib/types/logs.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- plugins/logging/operations.go
✅ Files skipped from review due to trivial changes (17)
- .gitignore
- ui/lib/constants/logs.ts
- .claude/skills/docs-writer/SKILL.md
- ui/app/workspace/logs/page.tsx
- core/schemas/context.go
- docs/openapi/openapi.json
- transports/bifrost-http/handlers/cache.go
- ui/lib/types/schemas.ts
- framework/logstore/tables.go
- transports/bifrost-http/handlers/logging.go
- framework/modelcatalog/sync.go
- docs/features/semantic-caching.mdx
- plugins/semanticcache/plugin_conversation_config_test.go
- transports/bifrost-http/handlers/middlewares.go
- plugins/semanticcache/plugin_normalization_test.go
- plugins/semanticcache/plugin_cross_cache_test.go
- plugins/semanticcache/plugin_no_store_test.go
🚧 Files skipped from review as they are similar to previous changes (30)
- docs/openapi/openapi.yaml
- core/schemas/context_test.go
- plugins/semanticcache/main_test.go
- plugins/semanticcache/state.go
- plugins/logging/main.go
- ui/lib/types/logs.ts
- docs/migration-guides/v1.5.0.mdx
- plugins/semanticcache/plugin_default_cache_key_test.go
- ui/lib/types/config.ts
- framework/vectorstore/weaviate.go
- docs/openapi/paths/management/cache.yaml
- framework/logstore/matviews.go
- plugins/semanticcache/plugin_no_mutation_test.go
- plugins/semanticcache/plugin_image_generation_test.go
- ui/components/filters/logsFilterSidebar.tsx
- framework/logstore/rdb.go
- plugins/semanticcache/plugin_vectorstore_test.go
- ui/app/workspace/logs/sheets/logDetailView.tsx
- transports/bifrost-http/handlers/cache_test.go
- plugins/semanticcache/plugin_api_test.go
- core/schemas/bifrost.go
- ui/app/workspace/config/views/pluginsForm.tsx
- plugins/semanticcache/plugin_streaming_test.go
- plugins/semanticcache/test_utils.go
- plugins/semanticcache/plugin_edge_cases_test.go
- plugins/semanticcache/stream.go
- plugins/semanticcache/plugin_embedding_test.go
- plugins/semanticcache/plugin_paths_test.go
- plugins/semanticcache/utils.go
- plugins/semanticcache/search.go
Pratham-Mishra04
force-pushed
the
04-30-fix_semantic_cache_fixes
branch
from
d03a719 to
88f9186
Compare
This was referenced May 8, 2026
Comment on lines
+460
to
+464
| func (plugin *Plugin) addNonStreamingResponse(ctx context.Context, responseID string, res *schemas.BifrostResponse, embedding []float32, metadata map[string]interface{}, ttl time.Duration) error { | ||
| responseData, err := json.Marshal(res) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal response: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
addSingleResponse snapshotted res synchronously via json.Marshal before spawning the goroutine, with an explicit comment explaining why: marshalling inside the goroutine races with downstream plugins (e.g. the logging plugin) that receive and mutate the same *BifrostResponse pointer after this PostLLMHook returns. That protection was removed when the function was renamed to addNonStreamingResponse and the marshal was moved inside it. With Go's race detector enabled, a typical pipeline where a logging plugin stamps telemetry after the semantic-cache plugin will trigger a data race on res.ExtraFields (or whichever field is mutated).
Suggested change
| func (plugin *Plugin) addNonStreamingResponse(ctx context.Context, responseID string, res *schemas.BifrostResponse, embedding []float32, metadata map[string]interface{}, ttl time.Duration) error { | |
| responseData, err := json.Marshal(res) | |
| if err != nil { | |
| return fmt.Errorf("failed to marshal response: %w", err) | |
| } | |
| func (plugin *Plugin) addNonStreamingResponse(ctx context.Context, responseID string, responseData []byte, embedding []float32, metadata map[string]interface{}, ttl time.Duration) error { |
Collaborator
Author
There was a problem hiding this comment.
No plugin in this codebase mutates the *BifrostResponse pointer after PostLLMHook returns — responses pass through plugins by reference but the only mutations happen synchronously inside the plugin's hook, which has already completed by the time this goroutine reads res. So marshalling inside the goroutine is safe; the prior comment about goroutine-vs-downstream-plugin race doesn't apply to the current plugin set. Keeping the current shape.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/app/workspace/logs/sheets/logDetailView.tsx (2)
1569-1591:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse null checks for numeric cache-debug fields.
These guards are truthy checks, so legitimate
0/0.0values forthreshold,similarity, andinput_tokensdisappear from the sheet. Please switch them to!= nullchecks so zero-valued diagnostics still render.Suggested fix
- {log.cache_debug.threshold && ( + {log.cache_debug.threshold != null && ( <LogEntryDetailsView className="w-full" label="Threshold" value={log.cache_debug.threshold || "-"} /> )} - {log.cache_debug.similarity && ( + {log.cache_debug.similarity != null && ( <LogEntryDetailsView className="w-full" label="Similarity Score" value={ log.cache_debug.similarity?.toFixed(2) || "-" } /> )} - {log.cache_debug.input_tokens && ( + {log.cache_debug.input_tokens != null && ( <LogEntryDetailsView className="w-full" label="Embedding Input Tokens" value={log.cache_debug.input_tokens} /> )}Also applies to: 1619-1623
🤖 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/logs/sheets/logDetailView.tsx` around lines 1569 - 1591, The current checks use truthy guards so numeric zeros are skipped; update the conditional guards around log.cache_debug.threshold, log.cache_debug.similarity, and log.cache_debug.input_tokens (and the duplicate block at the lines mentioned) to explicit null/undefined checks (e.g., use log.cache_debug.threshold != null) so 0 and 0.0 values still render in the LogEntryDetailsView components; keep the same label/value props and formatting (toFixed for similarity) when present.
1527-1631:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hide the new cache diagnostics on failed requests.
This panel is still nested under the surrounding
log.status === "success"branch, so any request that misses the cache and later fails only shows the header badge/cache ID and loses the detailed cache-debug fields. The new miss diagnostics need to render wheneverlog.cache_debugexists, not only on successful requests.🤖 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/logs/sheets/logDetailView.tsx` around lines 1527 - 1631, The cache diagnostics block is currently nested under the success-only branch so miss diagnostics disappear on failed requests; move the entire JSX that checks log.cache_debug (the fragment rendering BlockHeader, the grid and all LogEntryDetailsView / Badge usages) out of the surrounding log.status === "success" branch and render it whenever log.cache_debug is truthy; keep the existing checks inside (log.cache_debug.cache_hit, log.cache_debug.hit_type, provider_used, model_used, similarity, input_tokens, threshold) intact so the UI shows the same Hit/Miss layout and fields for both successful and failed requests.
🧹 Nitpick comments (3)
ui/app/workspace/config/views/pluginsForm.tsx (1)
343-351: ⚡ Quick winAdd
data-testidto the newly introduced interactive fields.The new controls (embedding model selector, vector namespace input, default cache key input) should expose stable test IDs for E2E selection.
Proposed patch
<ModelMultiselect inputId="embedding_model" + data-testid="semantic-cache-embedding-model-select" isSingleSelect provider={cacheConfig.provider || undefined} value={cacheConfig.embedding_model ?? ""} onChange={(model) => updateCacheConfigLocal({ embedding_model: model })} placeholder={cacheConfig.provider ? "Search or type an embedding model..." : "Select a provider first"} disabled={!cacheConfig.provider} /> ... <Input id="vector_store_namespace" + data-testid="semantic-cache-vector-namespace-input" type="text" placeholder="BifrostSemanticCachePlugin" value={cacheConfig.vector_store_namespace ?? ""} onChange={(e) => updateCacheConfigLocal({ vector_store_namespace: e.target.value })} /> ... <Input id="default_cache_key" + data-testid="semantic-cache-default-cache-key-input" type="text" placeholder="(none)" value={cacheConfig.default_cache_key ?? ""} onChange={(e) => updateCacheConfigLocal({ default_cache_key: e.target.value })} />As per coding guidelines: “Add data-testid to all new interactive elements in React components for E2E test compatibility” and use the
<entity>-<element>-<qualifier>convention.Also applies to: 440-461
🤖 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/config/views/pluginsForm.tsx` around lines 343 - 351, Add stable data-testid attributes to the new interactive fields so E2E tests can target them: for the ModelMultiselect component (symbol: ModelMultiselect, prop inputId="embedding_model", onChange handler updateCacheConfigLocal) add data-testid="cache-embedding-model-select"; likewise locate the vector namespace input (referenced by cacheConfig.vector_namespace or similar) and add data-testid="cache-vector-namespace-input", and the default cache key input (referenced by cacheConfig.default_cache_key or similar) add data-testid="cache-default-key-input" following the <entity>-<element>-<qualifier> convention; ensure the attributes are present on the rendered interactive element (select/input) and on both occurrences noted in the diff (around lines ~343 and ~440-461).plugins/semanticcache/plugin_core_test.go (1)
580-584: ⚡ Quick winRename this test to match the new contract.
These assertions now expect
Initto succeed and defer provider validation to request time, soTestInvalidProviderRejectionand its doc comment are misleading in CI output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/plugin_core_test.go` around lines 580 - 584, Rename the test function TestInvalidProviderRejection to reflect the new Init contract (e.g., TestInitSucceedsWithInvalidProviderOr TestInitDefersProviderValidation) and update its doc comment to state that provider validation is deferred to request time; ensure the test still calls Init(ctx, config, logger, mockStore) and asserts no error, and update any references to TestInvalidProviderRejection in the test file so names and comments match the new behavior.transports/bifrost-http/handlers/cache_test.go (1)
112-139: ⚡ Quick winAdd invalid-input parity tests for
clearCacheByKey.
clearCachealready verifies empty/missing route values, butclearCacheByKeycurrently doesn’t. Adding those two tests will prevent regressions where bad keys accidentally call the plugin or return wrong status codes.Proposed diff
func TestClearCacheByKey_OK(t *testing.T) { clearer := &fakeCacheClearer{} h := &CacheHandler{plugin: clearer} ctx := newCacheCtx("cacheKey", "session-42") h.clearCacheByKey(ctx) if got := ctx.Response.StatusCode(); got != fasthttp.StatusOK { t.Fatalf("expected 200, got %d body=%s", got, ctx.Response.Body()) } if len(clearer.keyCalls) != 1 || clearer.keyCalls[0] != "session-42" { t.Fatalf("expected ClearCacheForKey('session-42'), got %v", clearer.keyCalls) } } +func TestClearCacheByKey_RejectsEmptyKey(t *testing.T) { + clearer := &fakeCacheClearer{} + h := &CacheHandler{plugin: clearer} + + ctx := newCacheCtx("cacheKey", "") + h.clearCacheByKey(ctx) + + if got := ctx.Response.StatusCode(); got != fasthttp.StatusBadRequest { + t.Fatalf("expected 400 for empty key, got %d", got) + } + if len(clearer.keyCalls) != 0 { + t.Fatalf("expected no Clear calls on bad key, got %v", clearer.keyCalls) + } +} + +func TestClearCacheByKey_MissingUserValue(t *testing.T) { + clearer := &fakeCacheClearer{} + h := &CacheHandler{plugin: clearer} + + ctx := &fasthttp.RequestCtx{} + h.clearCacheByKey(ctx) + + if got := ctx.Response.StatusCode(); got != fasthttp.StatusBadRequest { + t.Fatalf("expected 400 when cacheKey user value missing, got %d", got) + } +} + func TestClearCacheByKey_PluginErrorReturns500(t *testing.T) { clearer := &fakeCacheClearer{ clearByKey: func(string) error { return errors.New("vector store down") }, }🤖 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/cache_test.go` around lines 112 - 139, Tests for clearCacheByKey are missing parity checks for invalid or missing route values; add two unit tests that mirror clearCache's behavior: one where the route key is an empty string and one where the route value is missing/empty (use newCacheCtx to create contexts like newCacheCtx("cacheKey", "") or a context without the "cacheKey"), invoke CacheHandler.clearCacheByKey with a fakeCacheClearer and assert that the plugin's ClearCacheForKey is not called (inspect fakeCacheClearer.keyCalls) and that the response status is fasthttp.StatusBadRequest; also keep an existing valid-key test to ensure no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/semanticcache/plugin_conversation_config_test.go`:
- Around line 66-67: The post-WaitForCache verification blocks that currently
call t.Skipf (the second-phase cache lookup checks following WaitForCache)
should not skip on upstream request errors; replace those t.Skipf calls with a
failing assertion (e.g., t.Fatalf or t.Fatalf-equivalent) so the test fails fast
when the second request fails. Locate the second-call blocks that occur after
WaitForCache in the test (the lines where you currently call t.Skipf("upstream
request error, skipping test: %v", err4) and the analogous occurrences) and
change them to a failing test call that reports the error and aborts the test
immediately.
In `@plugins/semanticcache/plugin_core_test.go`:
- Around line 466-479: The test's custom_ttl branch only checks
response2.ExtraFields.CacheDebug.CacheHit — change it to assert the configured
TTL was honored: either inspect response2.ExtraFields.CacheDebug for an
expiry/ttl field and compare it to the expected Config.TTL for the "custom_ttl"
case, or modify the test case for "custom_ttl" to use a very short TTL, wait
past that TTL, then re-request and assert CacheHit is false; update the
assertions around tt.expectedBehavior and response2.ExtraFields.CacheDebug to
validate TTL/expiry rather than just CacheHit.
In `@plugins/semanticcache/plugin_normalization_test.go`:
- Line 97: The tests currently call t.Skipf("upstream request error, skipping
test: %v", err1) unconditionally which can mask deterministic failures; replace
each unconditional skip (the t.Skipf calls referencing err1 at the four
occurrences) with logic that inspects the error and only skips for known
transient upstream conditions (e.g., network timeout, HTTP 429 rate-limit, or
5xx responses), otherwise fail the test (use t.Fatalf or t.Errorf). Update the
error handling where err1 is checked so transient cases call t.Skipf with a
clear message and non-transient cases call t.Fatalf including err1 so CI
surfaces real regressions.
In `@plugins/semanticcache/search.go`:
- Around line 406-440: The function stampCacheDebugForHit should early-return if
the extraFields parameter is nil to avoid panics when callers pass
cachedResponse.GetExtraFields() that can be nil; update stampCacheDebugForHit to
check `if extraFields == nil { return }` before dereferencing
extraFields.CacheDebug (function name: stampCacheDebugForHit; callers: places
that pass cachedResponse.GetExtraFields()).
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Around line 260-262: The JSX message concatenates the inline code element and
the word "request" so it renders as "embeddingrequest"; locate the JSX span
containing <code className="mx-1">embedding</code>request and insert a single
space between the closing </code> and "request" (e.g., change to ...</code>
request) to restore correct spacing.
In `@ui/app/workspace/logs/sheets/logDetailView.tsx`:
- Around line 848-858: The new CopyInlineButton usages for the request ID and
cache ID need stable test selectors: update the two instances where
CopyInlineButton is rendered (the one using log.id and the one using
log.cache_debug.cache_id) to pass a data-testid prop (e.g., "copy-request-id"
and "copy-cache-id"), and ensure the CopyInlineButton component signature
accepts and forwards that data-testid to the clickable element inside
(button/icon) so E2E tests can reliably target them.
---
Outside diff comments:
In `@ui/app/workspace/logs/sheets/logDetailView.tsx`:
- Around line 1569-1591: The current checks use truthy guards so numeric zeros
are skipped; update the conditional guards around log.cache_debug.threshold,
log.cache_debug.similarity, and log.cache_debug.input_tokens (and the duplicate
block at the lines mentioned) to explicit null/undefined checks (e.g., use
log.cache_debug.threshold != null) so 0 and 0.0 values still render in the
LogEntryDetailsView components; keep the same label/value props and formatting
(toFixed for similarity) when present.
- Around line 1527-1631: The cache diagnostics block is currently nested under
the success-only branch so miss diagnostics disappear on failed requests; move
the entire JSX that checks log.cache_debug (the fragment rendering BlockHeader,
the grid and all LogEntryDetailsView / Badge usages) out of the surrounding
log.status === "success" branch and render it whenever log.cache_debug is
truthy; keep the existing checks inside (log.cache_debug.cache_hit,
log.cache_debug.hit_type, provider_used, model_used, similarity, input_tokens,
threshold) intact so the UI shows the same Hit/Miss layout and fields for both
successful and failed requests.
---
Nitpick comments:
In `@plugins/semanticcache/plugin_core_test.go`:
- Around line 580-584: Rename the test function TestInvalidProviderRejection to
reflect the new Init contract (e.g., TestInitSucceedsWithInvalidProviderOr
TestInitDefersProviderValidation) and update its doc comment to state that
provider validation is deferred to request time; ensure the test still calls
Init(ctx, config, logger, mockStore) and asserts no error, and update any
references to TestInvalidProviderRejection in the test file so names and
comments match the new behavior.
In `@transports/bifrost-http/handlers/cache_test.go`:
- Around line 112-139: Tests for clearCacheByKey are missing parity checks for
invalid or missing route values; add two unit tests that mirror clearCache's
behavior: one where the route key is an empty string and one where the route
value is missing/empty (use newCacheCtx to create contexts like
newCacheCtx("cacheKey", "") or a context without the "cacheKey"), invoke
CacheHandler.clearCacheByKey with a fakeCacheClearer and assert that the
plugin's ClearCacheForKey is not called (inspect fakeCacheClearer.keyCalls) and
that the response status is fasthttp.StatusBadRequest; also keep an existing
valid-key test to ensure no regressions.
In `@ui/app/workspace/config/views/pluginsForm.tsx`:
- Around line 343-351: Add stable data-testid attributes to the new interactive
fields so E2E tests can target them: for the ModelMultiselect component (symbol:
ModelMultiselect, prop inputId="embedding_model", onChange handler
updateCacheConfigLocal) add data-testid="cache-embedding-model-select"; likewise
locate the vector namespace input (referenced by cacheConfig.vector_namespace or
similar) and add data-testid="cache-vector-namespace-input", and the default
cache key input (referenced by cacheConfig.default_cache_key or similar) add
data-testid="cache-default-key-input" following the
<entity>-<element>-<qualifier> convention; ensure the attributes are present on
the rendered interactive element (select/input) and on both occurrences noted in
the diff (around lines ~343 and ~440-461).
🪄 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: 653da5fe-a33b-4147-bee5-708a47323849
📒 Files selected for processing (55)
.claude/skills/docs-writer/SKILL.md.gitignorecore/schemas/bifrost.gocore/schemas/context.gocore/schemas/context_test.godocs/features/semantic-caching.mdxdocs/migration-guides/v1.5.0.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/cache.yamlframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goframework/modelcatalog/sync.goframework/vectorstore/weaviate.goplugins/logging/main.goplugins/logging/operations.goplugins/semanticcache/main.goplugins/semanticcache/main_test.goplugins/semanticcache/plugin_api_test.goplugins/semanticcache/plugin_cache_type_test.goplugins/semanticcache/plugin_conversation_config_test.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_cross_cache_test.goplugins/semanticcache/plugin_default_cache_key_test.goplugins/semanticcache/plugin_edge_cases_test.goplugins/semanticcache/plugin_embedding_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/plugin_nil_content_test.goplugins/semanticcache/plugin_no_mutation_test.goplugins/semanticcache/plugin_no_store_test.goplugins/semanticcache/plugin_normalization_test.goplugins/semanticcache/plugin_paths_test.goplugins/semanticcache/plugin_responses_test.goplugins/semanticcache/plugin_streaming_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/search.goplugins/semanticcache/state.goplugins/semanticcache/stream.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/cache_test.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/pluginsForm.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/components/filters/logsFilterSidebar.tsxui/lib/constants/logs.tsui/lib/store/apis/logsApi.tsui/lib/types/config.tsui/lib/types/logs.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- plugins/logging/operations.go
✅ Files skipped from review due to trivial changes (9)
- framework/modelcatalog/sync.go
- .gitignore
- ui/lib/types/logs.ts
- framework/logstore/tables.go
- plugins/semanticcache/state.go
- docs/openapi/openapi.json
- transports/bifrost-http/handlers/middlewares.go
- .claude/skills/docs-writer/SKILL.md
- docs/features/semantic-caching.mdx
🚧 Files skipped from review as they are similar to previous changes (29)
- plugins/semanticcache/main_test.go
- docs/openapi/openapi.yaml
- core/schemas/context_test.go
- plugins/semanticcache/plugin_streaming_test.go
- core/schemas/context.go
- docs/openapi/paths/management/cache.yaml
- plugins/semanticcache/plugin_no_mutation_test.go
- framework/logstore/matviews.go
- ui/app/workspace/logs/page.tsx
- ui/lib/constants/logs.ts
- framework/logstore/rdb.go
- ui/components/filters/logsFilterSidebar.tsx
- plugins/semanticcache/plugin_api_test.go
- plugins/semanticcache/plugin_image_generation_test.go
- core/schemas/bifrost.go
- plugins/semanticcache/stream.go
- plugins/semanticcache/plugin_cross_cache_test.go
- plugins/semanticcache/test_utils.go
- plugins/semanticcache/plugin_paths_test.go
- plugins/semanticcache/plugin_edge_cases_test.go
- ui/lib/types/schemas.ts
- plugins/semanticcache/plugin_no_store_test.go
- plugins/semanticcache/plugin_vectorstore_test.go
- ui/lib/types/config.ts
- plugins/semanticcache/plugin_nil_content_test.go
- plugins/semanticcache/main.go
- plugins/semanticcache/plugin_default_cache_key_test.go
- plugins/semanticcache/utils.go
- plugins/semanticcache/plugin_embedding_test.go
Comment on lines
+260
to
+262
| Requires at least one provider that supports embedding requests. Configure a built-in embedding provider, or enable the | ||
| <code className="mx-1">embedding</code>request type on a custom provider. | ||
| </span> |
Contributor
There was a problem hiding this comment.
Fix missing whitespace in the provider capability message.
On Line 261, </code>request renders as “embeddingrequest”. Add a space after </code>.
Proposed patch
- <code className="mx-1">embedding</code>request type on a custom provider.
+ <code className="mx-1">embedding</code> request type on a custom provider.🤖 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/config/views/pluginsForm.tsx` around lines 260 - 262, The
JSX message concatenates the inline code element and the word "request" so it
renders as "embeddingrequest"; locate the JSX span containing <code
className="mx-1">embedding</code>request and insert a single space between the
closing </code> and "request" (e.g., change to ...</code> request) to restore
correct spacing.
Collaborator
Author
There was a problem hiding this comment.
This file no longer exists in this branch — pluginsForm.tsx was removed during the semantic cache UI refactor (commits 1ebb11a83 and c94c276) and the relevant configuration has been split into cachingView.tsx and the other view files under ui/app/workspace/config/views/. The 'embeddingrequest' string is no longer present anywhere in the current code. Closing this out as no-longer-applicable.
Contributor
There was a problem hiding this comment.
@Pratham-Mishra04, thanks for the context! Since pluginsForm.tsx has been removed and the configuration has been restructured across cachingView.tsx and the other view files, this comment is indeed no longer applicable. The fix is moot.
🧠 Learnings used
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1326
File: ui/app/workspace/logs/views/imageView.tsx:22-39
Timestamp: 2026-01-14T09:00:53.418Z
Learning: In the Bifrost UI layer (ui/), ensure UI components do not incorporate streaming-awareness logic. Remove isStreaming props and any conditional rendering tied to streaming state from components, so they render data as provided by the API regardless of its streaming origin. This guidance applies to all TSX UI components under the ui directory.
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1719
File: ui/app/workspace/config/views/loggingView.tsx:7-10
Timestamp: 2026-02-21T09:47:17.525Z
Learning: In the UI codebase, prefer alias imports using `@/`... (e.g., `@/lib/store`, `@/lib/types/config`, `@/components/ui/button`) over relative imports. This file ui/app/workspace/config/views/loggingView.tsx should follow that pattern. Ensure path mapping is configured (e.g., tsconfig paths or webpack/babel module-resolver) so @ maps to the project root, and consider a lint rule to flag relative imports in UI files to maintain consistency.
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1716
File: ui/app/workspace/routing-rules/views/routingRulesEmptyState.tsx:26-41
Timestamp: 2026-02-22T11:22:16.817Z
Learning: When adding data-testid attributes in UI components, plan to include them in a dedicated PR that also updates related e2e tests, rather than sprinkling data-testid additions across feature PRs. This improves test reliability and makes test updates cohesive.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1725
File: ui/app/workspace/logs/layout.tsx:3-10
Timestamp: 2026-02-22T13:27:34.604Z
Learning: In the UI portion of the repo (ui/), import from the lib directory using the path alias '@/lib/*' as configured in tsconfig.json. This follows the established Next.js convention in this codebase. Do not replace '@/lib/*' with relative paths like '../../../lib/*'. Ensure tsconfig.json paths remain in sync and apply this alias import pattern to all UI TypeScript React files (ui/**/*.tsx).
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1735
File: ui/app/workspace/providers/views/addProviderDropdown.tsx:41-65
Timestamp: 2026-02-22T21:43:16.223Z
Learning: Maintain the data-testid conventions across the UI codebase: use add-{entity}-{element} (e.g., add-provider-btn, add-key-btn) and {entity}-{action}-{element} (e.g., key-cancel-btn) patterns consistently in UI components and E2E tests. Apply this in TSX files throughout the repository to ensure uniform selectors (including files like ui/app/workspace/providers/views/addProviderDropdown.tsx) and related tests.
Learnt from: jerkeyray
Repo: maximhq/bifrost PR: 1740
File: ui/lib/store/apis/pricingOverridesApi.ts:1-7
Timestamp: 2026-02-23T08:13:50.399Z
Learning: In the Bifrost codebase, prefer using the `@/lib` path alias for imports instead of relative paths from within the ui/lib directory. Apply this pattern broadly to all UI code under ui (files with .ts or .tsx extensions) to improve import readability and maintainability. For example, replace imports like ../../../lib/... with `@/lib/`... where appropriate.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1735
File: ui/app/workspace/governance/views/customerTable.tsx:19-23
Timestamp: 2026-02-24T20:31:46.598Z
Learning: During code reviews of UI components in the Bifrost repository, do not replace `@/lib/`* alias imports with relative paths in TSX files under the ui directory. The `@/lib/`* alias is the standard Next.js convention configured in tsconfig.json and is used consistently across the codebase; preserve this aliasing to maintain consistency and module resolution.
Learnt from: impoiler
Repo: maximhq/bifrost PR: 1968
File: ui/components/ui/markdown.tsx:11-16
Timestamp: 2026-03-09T09:37:50.229Z
Learning: In the Bifrost UI layer, general UI components under ui/ should be agnostic to streaming state (no isStreaming logic). Only library-wrapping components that directly drive a streaming library API (e.g., wrappers around streaming libraries like streamdown) may accept and pass through an isStreaming prop to configure the library. Do not couple standard components to streaming transport; ensure wrappers clearly document and map isStreaming to library props as needed.
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 2083
File: ui/components/prompts/components/emptyState.tsx:18-18
Timestamp: 2026-03-15T10:53:03.521Z
Learning: Enforce the 3-part data-testid convention: <entity>-<element>-<qualifier> for UI testids in Bifrost UI TSX components. The entity may be a hyphenated compound noun (e.g., empty-state); e.g., empty-state-create-prompt-link (entity=empty-state, element=create-prompt, qualifier=link) is valid. Do not flag 4-segment testids if the entity segment is itself a compound hyphenated name. Before raising an issue, verify sibling testids in the same file for consistency to avoid false positives.
Learnt from: danpiths
Repo: maximhq/bifrost PR: 2106
File: ui/app/workspace/routing-rules/views/routingRulesView.tsx:55-59
Timestamp: 2026-03-17T13:24:48.892Z
Learning: In paginated views, omit the API response object (e.g., rulesData) from useEffect dependencies if it is only used as a null-guard and meaningful changes surface through derived primitives (e.g., totalCount, offset). Since totalCount derives from rulesData?.total_count, changes to rulesData trigger updates via totalCount, and including the response object would cause unnecessary re-runs due to new object references on every poll. Apply this consistently to all paginated views (customers, teams, virtual keys, model limits, MCP clients, routing rules) in the UI.
Learnt from: sammaji
Repo: maximhq/bifrost PR: 2039
File: ui/app/workspace/config/views/compatibilityView.tsx:43-43
Timestamp: 2026-04-10T13:01:48.620Z
Learning: In the maximhq/bifrost UI, apply the lowercase-error-string convention only to Go code (not to the UI). For user-facing error toast messages in TypeScript/TSX UI components under `ui/` (e.g., `toast.error(...)`), use title/sentence case (e.g., `"Configuration not loaded"`) rather than forcing all-lowercase and avoiding trailing punctuation. Do not flag UI toast error strings for lowercase styling.
Learnt from: dominictayloruk
Repo: maximhq/bifrost PR: 2660
File: ui/app/workspace/config/views/cachingView.tsx:281-305
Timestamp: 2026-04-18T14:32:03.358Z
Learning: In maximhq/bifrost, inline required-field validation inside React save handlers is acceptable when it’s small (about 20 lines or less), applies to a fixed, limited set of providers/fields, and generates clear per-field toast error messages. Do not raise a review issue arguing to move this validation into Zod schemas in ui/lib/types/schemas.ts as long as equivalent server-side validation exists independently (i.e., the client-side checks are backed by server-side validation).
Learnt from: impoiler
Repo: maximhq/bifrost PR: 2789
File: ui/app/workspace/complexity-router/page.tsx:1-33
Timestamp: 2026-04-22T06:42:12.161Z
Learning: Within the Bifrost UI code under ui/ (TanStack React Router), do not introduce or recommend Next.js-specific constructs such as adding "use client" directives or any Next.js App Router conventions. Route files should use TanStack Router patterns (e.g., createFileRoute from tanstack/react-router). The `@/lib/`* path alias convention remains valid and is configured independently of Next.js.
Pratham-Mishra04
force-pushed
the
04-30-fix_semantic_cache_fixes
branch
from
88f9186 to
b344bb7
Compare
Comment on lines
669
to
+672
| func (plugin *Plugin) Cleanup() error { | ||
| plugin.waitGroup.Wait() | ||
| close(plugin.stopCh) | ||
| plugin.writersWg.Wait() | ||
| plugin.cleanupWg.Wait() |
Contributor
There was a problem hiding this comment.
Cleanup panics on a second call
close(plugin.stopCh) causes a "close of closed channel" panic if Cleanup is invoked more than once. The old implementation just called waitGroup.Wait(), which is safe to call repeatedly. Any lifecycle framework or test harness that calls Cleanup twice (or calls it on a partially-initialized plugin) will crash the process. A sync.Once guard around the close is the standard Go idiom to make this safe.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/semanticcache/plugin_cache_type_test.go (1)
74-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip transient provider failures on the miss-tolerant branches.
These requests are explicitly allowed to miss cache and fall through to the upstream model, so hard-failing on transient provider/network errors makes the suite flaky again. The assertions below already accept the miss path; these error branches should behave like the warm-up calls and
Skipf(...)instead.Suggested pattern
response2, err2 := setup.Client.ChatCompletionRequest(ctx2, similarRequest) if err2 != nil { - if err2.Error != nil { - t.Fatalf("Second request failed: %v", err2.Error.Message) - } else { - t.Fatalf("Second request failed: %v", err2) - } + t.Skipf("upstream request error, skipping test: %v", err2) }Apply the same change to the analogous semantic-miss branches later in this file.
Also applies to: 138-140, 233-235
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/semanticcache/plugin_cache_type_test.go` around lines 74 - 80, The test currently hard-fails on transient provider/network errors in the miss-tolerant branch (the block handling response2, err2 from setup.Client.ChatCompletionRequest), but these requests are allowed to fall through to upstream; change the failure logic to skip the test on transient errors instead of calling t.Fatalf — mimic the warm-up pattern and use t.Skipf with the error details when err2 indicates a transient/provider/network failure. Apply the same change to the analogous semantic-miss branches later in this file (the blocks around the response/err variables at the noted locations).
♻️ Duplicate comments (1)
framework/logstore/rdb.go (1)
195-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently turn invalid
cache_hit_typesinto an unfiltered query.If
filters.CacheHitTypesis non-empty but none of its values survive the allowlist, this branch adds no predicate at all, so stacked logs/stats/histogram requests likecache_hit_types=fooreturn full results instead of zero matches or a rejected filter.💡 Suggested fix
if len(filters.CacheHitTypes) > 0 { // Only keep allowed values to avoid passing arbitrary input into the JSON path expression. valid := make([]string, 0, len(filters.CacheHitTypes)) for _, t := range filters.CacheHitTypes { if t == "direct" || t == "semantic" { valid = append(valid, t) } } + if len(valid) == 0 { + return baseQuery.Where("1 = 0") + } if len(valid) > 0 { if s.db.Dialector.Name() == "postgres" { // Match the same loose-JSON guard used by aggregateCacheHits so the regex extract is safe. baseQuery = baseQuery.Where( "cache_debug IS NOT NULL AND cache_debug <> '' AND cache_debug ~ '^\\s*\\{.*\\}\\s*$' AND substring(cache_debug from '\"hit_type\"[[:space:]]*:[[:space:]]*\"([^\"]+)\"') IN ?", valid, )🤖 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 `@framework/logstore/rdb.go` around lines 195 - 217, The current handling of filters.CacheHitTypes allows an empty allowlist to silently drop the predicate and return all rows; after constructing valid from filters.CacheHitTypes, if len(filters.CacheHitTypes) > 0 and len(valid) == 0 you must add an explicit impossible predicate to baseQuery (e.g., WHERE 1=0) so the query returns zero rows for invalid input instead of being unfiltered; update the branch around valid and baseQuery (the block referencing filters.CacheHitTypes, valid, baseQuery and s.db.Dialector.Name()) to add that impossible-condition path for both Postgres and non-Postgres cases (or alternatively return an error) so invalid cache_hit_types do not produce full results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/semanticcache/plugin_cache_type_test.go`:
- Around line 286-295: Replace the flaky wall-clock assertions around
duration2/duration3/upperBoundForCacheLookup with a direct assertion that the
upstream provider/store was not invoked: use the test harness created by
NewTestSetup(t) (or the mock provider/store it exposes) to check its call-count
or a “was-called” flag after the cached lookups and assert it is zero; remove
the time-based comparisons and instead assert against the provider/mock's
invocation counter (e.g., mockProvider.CallCount == 0 or equivalent) for both
the direct-only and default-mode lookups to prove no upstream call was made.
---
Outside diff comments:
In `@plugins/semanticcache/plugin_cache_type_test.go`:
- Around line 74-80: The test currently hard-fails on transient provider/network
errors in the miss-tolerant branch (the block handling response2, err2 from
setup.Client.ChatCompletionRequest), but these requests are allowed to fall
through to upstream; change the failure logic to skip the test on transient
errors instead of calling t.Fatalf — mimic the warm-up pattern and use t.Skipf
with the error details when err2 indicates a transient/provider/network failure.
Apply the same change to the analogous semantic-miss branches later in this file
(the blocks around the response/err variables at the noted locations).
---
Duplicate comments:
In `@framework/logstore/rdb.go`:
- Around line 195-217: The current handling of filters.CacheHitTypes allows an
empty allowlist to silently drop the predicate and return all rows; after
constructing valid from filters.CacheHitTypes, if len(filters.CacheHitTypes) > 0
and len(valid) == 0 you must add an explicit impossible predicate to baseQuery
(e.g., WHERE 1=0) so the query returns zero rows for invalid input instead of
being unfiltered; update the branch around valid and baseQuery (the block
referencing filters.CacheHitTypes, valid, baseQuery and s.db.Dialector.Name())
to add that impossible-condition path for both Postgres and non-Postgres cases
(or alternatively return an error) so invalid cache_hit_types do not produce
full results.
🪄 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: 018cdddf-2f50-4fad-aadf-36d65fdcaf6c
📒 Files selected for processing (55)
.claude/skills/docs-writer/SKILL.md.gitignorecore/schemas/bifrost.gocore/schemas/context.gocore/schemas/context_test.godocs/features/semantic-caching.mdxdocs/migration-guides/v1.5.0.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/cache.yamlframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goframework/modelcatalog/sync.goframework/vectorstore/weaviate.goplugins/logging/main.goplugins/logging/operations.goplugins/semanticcache/main.goplugins/semanticcache/main_test.goplugins/semanticcache/plugin_api_test.goplugins/semanticcache/plugin_cache_type_test.goplugins/semanticcache/plugin_conversation_config_test.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_cross_cache_test.goplugins/semanticcache/plugin_default_cache_key_test.goplugins/semanticcache/plugin_edge_cases_test.goplugins/semanticcache/plugin_embedding_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_integration_test.goplugins/semanticcache/plugin_nil_content_test.goplugins/semanticcache/plugin_no_mutation_test.goplugins/semanticcache/plugin_no_store_test.goplugins/semanticcache/plugin_normalization_test.goplugins/semanticcache/plugin_paths_test.goplugins/semanticcache/plugin_responses_test.goplugins/semanticcache/plugin_streaming_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/search.goplugins/semanticcache/state.goplugins/semanticcache/stream.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/cache.gotransports/bifrost-http/handlers/cache_test.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/handlers/middlewares.goui/app/workspace/config/views/pluginsForm.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/components/filters/logsFilterSidebar.tsxui/lib/constants/logs.tsui/lib/store/apis/logsApi.tsui/lib/types/config.tsui/lib/types/logs.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (1)
- plugins/logging/operations.go
✅ Files skipped from review due to trivial changes (10)
- .gitignore
- docs/openapi/openapi.json
- ui/lib/store/apis/logsApi.ts
- ui/lib/types/schemas.ts
- core/schemas/context.go
- .claude/skills/docs-writer/SKILL.md
- framework/logstore/tables.go
- docs/features/semantic-caching.mdx
- transports/bifrost-http/handlers/middlewares.go
- framework/modelcatalog/sync.go
🚧 Files skipped from review as they are similar to previous changes (39)
- ui/lib/constants/logs.ts
- ui/lib/types/logs.ts
- ui/components/filters/logsFilterSidebar.tsx
- plugins/semanticcache/state.go
- framework/logstore/matviews.go
- plugins/semanticcache/plugin_default_cache_key_test.go
- docs/openapi/paths/management/cache.yaml
- plugins/semanticcache/plugin_streaming_test.go
- ui/app/workspace/logs/page.tsx
- core/schemas/bifrost.go
- ui/lib/types/config.ts
- plugins/semanticcache/plugin_no_store_test.go
- ui/app/workspace/logs/sheets/logDetailView.tsx
- plugins/semanticcache/plugin_no_mutation_test.go
- transports/bifrost-http/handlers/logging.go
- core/schemas/context_test.go
- plugins/semanticcache/plugin_cross_cache_test.go
- docs/openapi/openapi.yaml
- framework/vectorstore/weaviate.go
- docs/migration-guides/v1.5.0.mdx
- plugins/semanticcache/test_utils.go
- plugins/semanticcache/plugin_image_generation_test.go
- plugins/semanticcache/plugin_vectorstore_test.go
- plugins/semanticcache/stream.go
- plugins/semanticcache/plugin_nil_content_test.go
- plugins/semanticcache/plugin_responses_test.go
- transports/bifrost-http/handlers/cache_test.go
- plugins/semanticcache/plugin_normalization_test.go
- plugins/semanticcache/plugin_api_test.go
- plugins/semanticcache/plugin_edge_cases_test.go
- plugins/semanticcache/main_test.go
- plugins/semanticcache/plugin_integration_test.go
- plugins/semanticcache/plugin_embedding_test.go
- plugins/semanticcache/utils.go
- plugins/semanticcache/plugin_paths_test.go
- ui/app/workspace/config/views/pluginsForm.tsx
- plugins/logging/main.go
- plugins/semanticcache/main.go
- plugins/semanticcache/search.go
Comment on lines
+286
to
+295
| // Both lookups hit direct cache so both must be substantially faster than | ||
| // a real upstream call. Compare against an upper bound rather than each | ||
| // other (relative comparisons flake under CI load); 1s is generous and | ||
| // still proves a cached lookup didn't silently hit the network. | ||
| const upperBoundForCacheLookup = 1 * time.Second | ||
| if duration2 > upperBoundForCacheLookup { | ||
| t.Errorf("direct-only cache lookup took %v, expected < %v (provider likely called)", duration2, upperBoundForCacheLookup) | ||
| } | ||
| if duration3 > upperBoundForCacheLookup { | ||
| t.Errorf("default-mode cache lookup took %v, expected < %v (provider likely called)", duration3, upperBoundForCacheLookup) |
Contributor
There was a problem hiding this comment.
The 1s wall-clock bound will still flake under CI load.
This is an integration path using NewTestSetup(t), and the file already opts into parallel execution. Even a healthy cache hit can cross this threshold on a busy runner, so the failure mode becomes scheduler/load noise instead of a cache regression. It would be more reliable to assert “no upstream call” directly via cache-debug/store/provider instrumentation than by elapsed time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/semanticcache/plugin_cache_type_test.go` around lines 286 - 295,
Replace the flaky wall-clock assertions around
duration2/duration3/upperBoundForCacheLookup with a direct assertion that the
upstream provider/store was not invoked: use the test harness created by
NewTestSetup(t) (or the mock provider/store it exposes) to check its call-count
or a “was-called” flag after the cached lookups and assert it is zero; remove
the time-based comparisons and instead assert against the provider/mock's
invocation counter (e.g., mockProvider.CallCount == 0 or equivalent) for both
the direct-only and default-mode lookups to prove no upstream call was made.
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
3 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
This PR refactors the semantic cache plugin to simplify its internal state management, improves cache lookup correctness, and adds a new
cache_hit_typesfilter to the logs API and UI. The direct cache lookup path is now a single deterministic point-fetch by a UUIDv5directCacheID(replacing the previous dual-path of chunk lookup + legacy metadata scan), and several context keys are consolidated. The UI gains a "Local Caching" filter sidebar section and cache hit type badges in the log detail view.Changes
Semantic cache plugin refactor:
performDirectChunkLookup+performLegacyDirectSearch) with a singleperformDirectSearchthat does an O(1)GetChunkby deterministicdirectCacheID(UUIDv5 derived from provider, model, cacheKey, requestHash, paramsHash).generateDirectCacheIDnow returns an error instead of silently falling back to a string concatenation, making failures explicit.request_hashis no longer stored as a top-level metadata field; it is encoded into thedirectCacheIDinstead.directCacheIDKey,paramsHashKey,embeddingsKey,embeddingsInputTokensKey), removing stale keys likerequestIDKey,requestHashKey,isCacheHitKey, andcacheHitTypeKey.shouldSkipCachingis extracted into its own method; cache-hit detection now readsCacheDebug.CacheHitfrom the response rather than a context flag.buildUnifiedMetadatano longer acceptsrequestHashas a parameter.addSingleResponserenamed toaddNonStreamingResponse.StreamAccumulatorfieldsHasError,FinalTimestamp, andFinishReasononStreamChunkare removed; error streams are handled by early return inPostLLMHook.ctx.Done()to prevent goroutine leaks on dropped consumers.runStreamCleanupLoopgoroutine (started byInit, stopped byCleanupviastopCh) replaces the one-shot cleanup call, periodically reaping stale stream accumulators.buildResponseFromResultnow acceptsthreshold,similarity, andinputTokensas pointers, andattachCacheDebugis extracted as a shared helper for both streaming and non-streaming paths.isExpiredEntryis extracted as a standalone function.chunkSortKeyreplaces the large inline sort comparator inprocessAccumulatedStream.hashSortedSet/sortedStringSetto prevent MCP's randomized map iteration from perturbing the request hash.extractAttachmentsForCachingis extracted so attachment URLs are included in the cache key metadata rather than the embedding text.extractTextForEmbeddingno longer returns aparamsHash; callers compute it once viabuildRequestMetadataForCaching+hashMap.generateEmbeddingmoved fromutils.gotosearch.go.generateRequestHashnow accepts prebuilt metadata to avoid recomputing it.removeFieldno longer mutates the input slice's backing array.PronunciationDictionaryLocators,TimestampGranularities,Include,AdditionalFormats, andInputImagesto their respective parameter metadata extractors.semantic_cache_*tosemantic_cache-*(underscore → hyphen separator after the plugin prefix).SelectFieldsno longer includesrequest_hash.VectorStorePropertiesno longer includes arequest_hashentry.CacheByModelandCacheByProviderdefault-value log messages added.Log filtering —
cache_hit_types:CacheHitTypes []stringtoSearchFiltersinframework/logstore/tables.go.applyFiltersinrdb.goapplies a JSON path filter oncache_debugfor both SQLite (json_extract) and PostgreSQL (substringregex) dialects, restricted to the allowlist["direct", "semantic"].canUseMatViewFiltersexcludes queries withCacheHitTypesset from the materialized-view fast path.getLogs,getLogsStats,parseHistogramFilters) parse acache_hit_typescomma-separated query parameter.UI:
LogsFilterSidebarwith checkboxes for "Direct cache" and "Semantic cache".cache_hit_typesis added to URL state, filter state, and thebuildFilterParamsAPI helper.cache_debug.hit_type.EmbeddingSupportedProvidersfor built-ins;custom_provider_config.allowed_requests.embeddingfor custom providers), shows an error message when no embedding provider is configured, and disables the toggle accordingly.ModelMultiselect(single-select mode) scoped to the selected provider.EmbeddingSupportedProvidersconstant added toui/lib/constants/logs.ts.Misc:
CorsMiddlewareand an auth debug log are commented out.transports/bifrost-http/v1.5.xadded to.gitignore.core/schemas/bifrost.goandframework/modelcatalog/sync.go.sync.goadded.Type of change
Affected areas
How to test
hit_typeincache_debug./logs?cache_hit_types=directand/logs?cache_hit_types=semanticand confirm only matching entries are returned.Breaking changes
The public semantic cache context key names have changed from
semantic_cache_*tosemantic_cache-*. Any caller settingCacheKey,CacheTTLKey,CacheThresholdKey,CacheTypeKey, orCacheNoStoreKeyvia the old string values will no longer be recognized by the plugin. Update all call sites to use the exported constants from the plugin package rather than raw string literals.request_hashis no longer stored as a top-level metadata field in the vector store. Existing cache entries written by prior versions will not be found by the new direct-search path (they will be treated as misses and re-populated).ClearCacheForRequestIDis documented as currently broken for entries written by the new direct-search path; callers should not rely on it until the TODO is resolved.Related issues
N/A
Security considerations
The
CacheHitTypesfilter allowlists values to"direct"and"semantic"before interpolating them into SQL, preventing arbitrary input from reaching the JSON path expression.Checklist
docs/contributing/README.mdand followed the guidelines