Conversation
|
|
This was referenced Apr 27, 2026
feat: add opt-in per-request overrides for content logging and raw request/response visibility
#3066
Merged
Collaborator
Author
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe semantic-cache plugin no longer stores provider API keys or an internal Bifrost client. An EmbeddingRequestExecutor callback and setter were added; server bootstrap/reload wires the executor from the global Bifrost client. Config/schema/UI/types/tests were updated to remove Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant Server as Server
participant Plugin as SemanticCachePlugin
participant Bifrost as BifrostClient
UI->>Server: Upload plugin config (no keys) / Requests
Server->>Plugin: Instantiate plugin
Server->>Plugin: SetEmbeddingRequestExecutor(Bifrost.EmbeddingRequest)
Plugin->>Bifrost: EmbeddingRequest(...) via injected executor
Bifrost-->>Plugin: EmbeddingResponse
Plugin->>Plugin: Semantic search / cache logic
Plugin-->>UI: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Confidence Score: 5/5Safe to merge; all findings are P2 quality-of-life suggestions with no blocking defects. The refactor is clean and well-scoped. The ReloadPlugin executor-ordering issue from the previous review thread is correctly addressed. Both P2 findings (warning-only validation at startup, missing test assertion) are non-blocking and acknowledged in the PR description. No regressions in the core plugin pipeline path. transports/bifrost-http/lib/config.go — the warning-only provider validation at line 2627 merits a second look if hard startup errors are preferred. Important Files Changed
Reviews (4): Last reviewed commit: "refactor: use global bifrost client in s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/lib/config.go (1)
4800-4805:⚠️ Potential issue | 🟡 MinorNormalize provider casing before the global lookup.
This only trims whitespace, but the provider loader normalizes names to lowercase. A config like
provider: "OpenAI"will failGetProviderConfigRaw(...)even when the global provider is configured. Lowercasing here keeps semantic-cache plugin config behavior consistent with the rest of the loader.Suggested fix
provider, ok := providerVal.(string) if !ok { return fmt.Errorf("semantic_cache plugin 'provider' field must be a string, got %T", providerVal) } - provider = strings.TrimSpace(provider) + provider = strings.ToLower(strings.TrimSpace(provider)) configMap["provider"] = provider🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 4800 - 4805, The provider string is trimmed but not normalized, causing mismatches with the global provider lookup; update the provider handling in the block that processes providerVal (the provider variable and configMap assignment in config.go) to convert provider to lowercase (e.g., strings.ToLower(provider)) after trimming and before storing it in configMap so subsequent calls like GetProviderConfigRaw(...) will find the provider regardless of input casing.plugins/semanticcache/main.go (1)
398-421:⚠️ Potential issue | 🟠 MajorMissing executor silently disables semantic caching.
If semantic mode is configured but
SetEmbeddingRequestExecutor(...)was never called, this branch is skipped and the request continues as if nothing is wrong. Later,PostLLMHookcan end up storingnilembeddings, so semantic entries never warm correctly and vector stores that require vectors will reject the write. Please fail fast or emit a clear warning when semantic mode runs without an executor.Suggested fix
- if performSemanticSearch && plugin.embeddingRequestExecutor != nil { + if performSemanticSearch { + if plugin.embeddingRequestExecutor == nil { + return req, nil, fmt.Errorf("semantic cache embedding executor is not configured") + } if req.EmbeddingRequest != nil || req.TranscriptionRequest != nil { plugin.logger.Debug(PluginLoggerPrefix + " Skipping semantic search for embedding/transcription input") // For vector stores that require vectors, set a zero vector placeholder @@ - } else if !performSemanticSearch && plugin.store.RequiresVectors() && plugin.embeddingRequestExecutor != nil { + } else if !performSemanticSearch && plugin.store.RequiresVectors() && plugin.embeddingRequestExecutor != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/main.go` around lines 398 - 421, The code allows semantic mode to proceed without an embedding executor, causing silent failures and nil embeddings; add a guard when performSemanticSearch is true (and before using plugin.performSemanticSearch or letting the request proceed) to check plugin.embeddingRequestExecutor != nil and if nil either return an error (fail fast) or emit a clear plugin.logger.Warn/error indicating SetEmbeddingRequestExecutor was not called and semantic caching is disabled; ensure this check is applied alongside plugin.store.RequiresVectors() logic so PostLLMHook won't store nil embeddings and vector stores that require vectors are not written to.
🧹 Nitpick comments (2)
plugins/semanticcache/plugin_core_test.go (1)
542-547: Re-add request-time coverage for the new validation boundary.These assertions only prove that
Init(...)no longer rejects the config. They do not verify the behavior this PR actually moved to runtime: executor wiring plus request-time provider validation. A small stubEmbeddingRequestExecutortest here would catch regressions where unsupported providers fail too late or supported providers never reach embedding execution.Also applies to: 569-572
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/plugin_core_test.go` around lines 542 - 547, Add a test that exercises request-time provider validation by stubbing an EmbeddingRequestExecutor and invoking the executor path after Init: keep using Init(ctx, config, logger, mockStore) to ensure init succeeds, then create a minimal EmbeddingRequestExecutor stub (or mock) and call the embedding execution function with the same config/provider to assert that supported providers run the executor and unsupported providers return a validation error at request time; update the existing test blocks around Init (including the similar assertions at the other location mentioned) to perform this executor-call check so the test verifies both wiring and runtime provider validation.transports/bifrost-http/lib/semantic_cache_config_test.go (1)
31-49: Assert stalekeyshandling explicitly in this stale-field test.Line 37 seeds stale
keys, but the test only verifiesembedding_modelremoval (Line 47-Line 48). Add an explicitkeysassertion so the test name and coverage stay aligned.✅ Suggested test assertion addition
configMap, ok := pluginConfig.Config.(map[string]interface{}) require.True(t, ok) _, hasEmbeddingModel := configMap["embedding_model"] require.False(t, hasEmbeddingModel, "direct-only mode should remove stale embedding_model") +_, hasKeys := configMap["keys"] +require.False(t, hasKeys, "direct-only mode should remove stale keys")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/semantic_cache_config_test.go` around lines 31 - 49, The test TestValidateSemanticCacheConfig_DirectOnlyModeRemovesStaleProviderBackedFields seeds a stale "keys" entry but only asserts removal of "embedding_model"; update the test to also assert handling of "keys" after calling Config.ValidateSemanticCacheConfig by inspecting pluginConfig.Config (cast to configMap) and verifying that the "keys" entry has been removed or is emptied (e.g., no "keys" key present or its value is an empty slice), so the test name and coverage match; reference Config.ValidateSemanticCacheConfig, pluginConfig.Config, and the "keys" map entry when adding the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/features/semantic-caching.mdx`:
- Around line 182-183: Replace the inaccurate sentence "Changes are persisted
immediately but take effect on the next Bifrost restart, because plugins are
loaded at startup." with copy that clarifies behavior: state that changes are
persisted immediately, and that plugin updates applied via the API path are
reloaded and take effect without a restart while other plugin changes may still
require a restart; use explicit phrasing like "Changes are persisted
immediately; plugin updates applied via the API path are reloaded and take
effect without restarting Bifrost, whereas other plugin changes may still
require a restart because plugins are loaded at startup." to avoid misleading
operators.
In `@plugins/semanticcache/utils.go`:
- Line 77: The call to plugin.embeddingRequestExecutor may be nil and can cause
a runtime panic; before invoking embeddingRequestExecutor with (embeddingCtx,
embeddingReq) check that plugin.embeddingRequestExecutor != nil and handle the
nil case (return an error, log and return, or fall back to a default executor)
so that response/err are only obtained from a valid function; update the code
around the line that assigns response, err :=
plugin.embeddingRequestExecutor(embeddingCtx, embeddingReq) to perform this
guard and consistent error handling.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2623-2626: The semantic cache validation currently only runs for
plugins from the config store; update the code path that processes file-backed
plugins (where configData.Plugins is iterated/merged by mergePlugins) to call
config.ValidateSemanticCacheConfig(pluginConfig) for any plugin whose
plugin.Name == semanticcache.PluginName before accepting/upserting it. In
practice, locate the logic handling configData.Plugins/mergePlugins and add the
same validation and logger.Warn(...) used in the config-store branch so
file-backed semantic_cache entries are validated and rejected/warned
consistently.
In `@transports/bifrost-http/server/server.go`:
- Around line 972-977: The code sets the EmbeddingRequestExecutor on the plugin
instance loaded from s.Config instead of the newly reloaded instance `plugin`;
change it to type-assert the local `plugin` to *semanticcache.Plugin (or use the
appropriate helper to cast the reloaded instance) and call
SetEmbeddingRequestExecutor on that instance (e.g. if p, ok :=
plugin.(*semanticcache.Plugin); ok {
p.SetEmbeddingRequestExecutor(s.Client.EmbeddingRequest) }) so the newly
instantiated plugin returned by SyncLoadedPlugin gets configured.
---
Outside diff comments:
In `@plugins/semanticcache/main.go`:
- Around line 398-421: The code allows semantic mode to proceed without an
embedding executor, causing silent failures and nil embeddings; add a guard when
performSemanticSearch is true (and before using plugin.performSemanticSearch or
letting the request proceed) to check plugin.embeddingRequestExecutor != nil and
if nil either return an error (fail fast) or emit a clear
plugin.logger.Warn/error indicating SetEmbeddingRequestExecutor was not called
and semantic caching is disabled; ensure this check is applied alongside
plugin.store.RequiresVectors() logic so PostLLMHook won't store nil embeddings
and vector stores that require vectors are not written to.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4800-4805: The provider string is trimmed but not normalized,
causing mismatches with the global provider lookup; update the provider handling
in the block that processes providerVal (the provider variable and configMap
assignment in config.go) to convert provider to lowercase (e.g.,
strings.ToLower(provider)) after trimming and before storing it in configMap so
subsequent calls like GetProviderConfigRaw(...) will find the provider
regardless of input casing.
---
Nitpick comments:
In `@plugins/semanticcache/plugin_core_test.go`:
- Around line 542-547: Add a test that exercises request-time provider
validation by stubbing an EmbeddingRequestExecutor and invoking the executor
path after Init: keep using Init(ctx, config, logger, mockStore) to ensure init
succeeds, then create a minimal EmbeddingRequestExecutor stub (or mock) and call
the embedding execution function with the same config/provider to assert that
supported providers run the executor and unsupported providers return a
validation error at request time; update the existing test blocks around Init
(including the similar assertions at the other location mentioned) to perform
this executor-call check so the test verifies both wiring and runtime provider
validation.
In `@transports/bifrost-http/lib/semantic_cache_config_test.go`:
- Around line 31-49: The test
TestValidateSemanticCacheConfig_DirectOnlyModeRemovesStaleProviderBackedFields
seeds a stale "keys" entry but only asserts removal of "embedding_model"; update
the test to also assert handling of "keys" after calling
Config.ValidateSemanticCacheConfig by inspecting pluginConfig.Config (cast to
configMap) and verifying that the "keys" entry has been removed or is emptied
(e.g., no "keys" key present or its value is an empty slice), so the test name
and coverage match; reference Config.ValidateSemanticCacheConfig,
pluginConfig.Config, and the "keys" map entry when adding the assertion.
🪄 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: ecb6a778-164b-435f-9f96-b7700be0fec9
📒 Files selected for processing (15)
docs/features/semantic-caching.mdxplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/semantic_cache_config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/pluginsForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (5)
- plugins/semanticcache/plugin_vectorstore_test.go
- plugins/semanticcache/plugin_image_generation_test.go
- transports/config.schema.json
- ui/lib/types/config.ts
- ui/lib/types/schemas.ts
Pratham-Mishra04
force-pushed
the
04-27-docs_default_provider_selection_doc_updates
branch
from
c8cc2e4 to
da8b364
Compare
Pratham-Mishra04
force-pushed
the
04-27-refactor_use_global_bifrost_client_in_semmantic_cache_plugin
branch
from
f9dbc08 to
7fc9839
Compare
This was referenced Apr 27, 2026
fix: allow self-looping chain rules to continue evaluating subsequent rules instead of halting
#3082
Merged
akshaydeo
requested changes
Apr 28, 2026
Contributor
Merge activity
|
Pratham-Mishra04
force-pushed
the
04-27-docs_default_provider_selection_doc_updates
branch
from
da8b364 to
fe670d3
Compare
Pratham-Mishra04
force-pushed
the
04-27-refactor_use_global_bifrost_client_in_semmantic_cache_plugin
branch
from
7fc9839 to
92ddedd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/lib/config.go (1)
2625-2637:⚠️ Potential issue | 🟠 MajorSemantic-cache validation is still skipped for config-file plugin merge path.
Line 2626 applies validation only for DB-loaded plugins, but plugins entering through
mergePlugins(...)are still accepted withoutValidateSemanticCacheConfig. This keeps file-backed semantic-cache configs on a different (less safe) path.Suggested fix
func mergePlugins(ctx context.Context, config *Config, configData *ConfigData) { logger.Debug("processing plugins from config file") + validateSemanticCache := func(plugin *schemas.PluginConfig) { + if plugin.Name == semanticcache.PluginName { + if err := config.ValidateSemanticCacheConfig(plugin); err != nil { + logger.Warn("failed to validate semantic cache config: %v", err) + } + } + } + if len(config.PluginConfigs) == 0 { logger.Debug("no plugins found in store, using plugins from config file") + for _, plugin := range configData.Plugins { + validateSemanticCache(plugin) + } config.PluginConfigs = configData.Plugins } else { // Merge new plugins and update if version is higher for _, plugin := range configData.Plugins { + validateSemanticCache(plugin) if plugin.Version == nil { plugin.Version = bifrost.Ptr(int16(1)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/config.go` around lines 2625 - 2637, The semantic-cache config validation is only run for DB-loaded plugins and is missing for plugins merged via mergePlugins; update the mergePlugins flow (or immediately after mergePlugins returns) to iterate over the merged plugin entries and call config.ValidateSemanticCacheConfig(pluginConfig) for any plugin whose Name == semanticcache.PluginName, logging warnings on validation errors (use the same logger pattern as the DB path); ensure you reference the same symbols: mergePlugins, config.ValidateSemanticCacheConfig, semanticcache.PluginName and config.PluginConfigs so the merged file-backed plugins go through the same validation path.
🧹 Nitpick comments (1)
plugins/semanticcache/plugin_core_test.go (1)
511-513: Rename test to match new behavior.
TestInvalidProviderRejectionnow validates the opposite behavior (Init acceptance). Renaming it will prevent future confusion.✏️ Suggested rename
-// TestInvalidProviderRejection tests that providers without embedding support are rejected during initialization -func TestInvalidProviderRejection(t *testing.T) { +// TestInitDoesNotRejectUnsupportedEmbeddingProviders verifies provider checks are deferred to request time. +func TestInitDoesNotRejectUnsupportedEmbeddingProviders(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/semanticcache/plugin_core_test.go` around lines 511 - 513, The test function named TestInvalidProviderRejection now verifies the opposite (that Init accepts the provider), so rename the test symbol TestInvalidProviderRejection to a name reflecting the new behavior (e.g., TestProviderInitAcceptance or TestValidProviderAcceptance) and update the test's comment/header string accordingly; search for any occurrences of TestInvalidProviderRejection (the test function declaration and any references) and update them to the new name so the test name and description match the implemented assertion about Init acceptance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/semanticcache/main.go`:
- Line 421: The branch that sets the zero-vector placeholder is incorrectly
gated on plugin.embeddingRequestExecutor != nil; remove the executor nil check
so the zero-vector placeholder is initialized whenever !performSemanticSearch &&
plugin.store.RequiresVectors() is true (leave the executor check only where
actual embedding calls occur). Update the conditional(s) around the zero-vector
placeholder setup (the branch using performSemanticSearch,
plugin.store.RequiresVectors(), and plugin.embeddingRequestExecutor) so the
placeholder logic runs regardless of embeddingRequestExecutor, while preserving
executor-dependent code paths for real embedding requests elsewhere in the same
function.
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2625-2637: The semantic-cache config validation is only run for
DB-loaded plugins and is missing for plugins merged via mergePlugins; update the
mergePlugins flow (or immediately after mergePlugins returns) to iterate over
the merged plugin entries and call
config.ValidateSemanticCacheConfig(pluginConfig) for any plugin whose Name ==
semanticcache.PluginName, logging warnings on validation errors (use the same
logger pattern as the DB path); ensure you reference the same symbols:
mergePlugins, config.ValidateSemanticCacheConfig, semanticcache.PluginName and
config.PluginConfigs so the merged file-backed plugins go through the same
validation path.
---
Nitpick comments:
In `@plugins/semanticcache/plugin_core_test.go`:
- Around line 511-513: The test function named TestInvalidProviderRejection now
verifies the opposite (that Init accepts the provider), so rename the test
symbol TestInvalidProviderRejection to a name reflecting the new behavior (e.g.,
TestProviderInitAcceptance or TestValidProviderAcceptance) and update the test's
comment/header string accordingly; search for any occurrences of
TestInvalidProviderRejection (the test function declaration and any references)
and update them to the new name so the test name and description match the
implemented assertion about Init acceptance.
🪄 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: 836d8e41-e679-4fae-8b85-9b53670dda95
📒 Files selected for processing (15)
docs/features/semantic-caching.mdxplugins/semanticcache/main.goplugins/semanticcache/plugin_core_test.goplugins/semanticcache/plugin_image_generation_test.goplugins/semanticcache/plugin_vectorstore_test.goplugins/semanticcache/test_utils.goplugins/semanticcache/utils.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/semantic_cache_config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/pluginsForm.tsxui/lib/types/config.tsui/lib/types/schemas.ts
💤 Files with no reviewable changes (4)
- plugins/semanticcache/plugin_vectorstore_test.go
- plugins/semanticcache/plugin_image_generation_test.go
- ui/lib/types/config.ts
- transports/config.schema.json
✅ Files skipped from review due to trivial changes (2)
- transports/bifrost-http/handlers/plugins.go
- ui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/features/semantic-caching.mdx
- transports/bifrost-http/lib/semantic_cache_config_test.go
| return req, shortCircuit, nil | ||
| } | ||
| } else if !performSemanticSearch && plugin.store.RequiresVectors() && plugin.client != nil { | ||
| } else if !performSemanticSearch && plugin.store.RequiresVectors() && plugin.embeddingRequestExecutor != nil { |
Contributor
There was a problem hiding this comment.
Direct-only vector placeholder should not depend on executor presence.
The branch that sets the zero-vector placeholder doesn’t call the executor, but it is currently gated by plugin.embeddingRequestExecutor != nil. That can suppress vector-required storage prep in direct-only paths when executor wiring is absent.
🔧 Proposed fix
- } else if !performSemanticSearch && plugin.store.RequiresVectors() && plugin.embeddingRequestExecutor != nil {
+ } else if !performSemanticSearch && plugin.store.RequiresVectors() {Also applies to: 428-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/semanticcache/main.go` at line 421, The branch that sets the
zero-vector placeholder is incorrectly gated on plugin.embeddingRequestExecutor
!= nil; remove the executor nil check so the zero-vector placeholder is
initialized whenever !performSemanticSearch && plugin.store.RequiresVectors() is
true (leave the executor check only where actual embedding calls occur). Update
the conditional(s) around the zero-vector placeholder setup (the branch using
performSemanticSearch, plugin.store.RequiresVectors(), and
plugin.embeddingRequestExecutor) so the placeholder logic runs regardless of
embeddingRequestExecutor, while preserving executor-dependent code paths for
real embedding requests elsewhere in the same function.
akshaydeo
changed the base branch from
04-27-docs_default_provider_selection_doc_updates
to
graphite-base/3079
akshaydeo
force-pushed
the
04-27-refactor_use_global_bifrost_client_in_semmantic_cache_plugin
branch
from
92ddedd to
ef2e204
Compare
akshaydeo
deleted the
04-27-refactor_use_global_bifrost_client_in_semmantic_cache_plugin
branch
18 tasks
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
The semantic cache plugin previously managed its own internal Bifrost client and required API keys to be specified directly in the plugin config. This PR removes that self-contained client and replaces it with an injected
EmbeddingRequestExecutor— a function reference pointing to the global Bifrost client'sEmbeddingRequestmethod. As a result, provider API keys are inherited automatically from the global provider configuration and no longer need to be (or can be) specified inside the plugin config.Changes
Keys []schemas.Keyfield fromsemanticcache.Configand all related JSON unmarshalling, test fixtures, and UI type definitions.PluginAccountstruct and the self-containedbifrost.Bifrostclient that the plugin previously initialized for embedding requests.EmbeddingRequestExecutoras a function type (func(*schemas.BifrostContext, *schemas.BifrostEmbeddingRequest) (*schemas.BifrostEmbeddingResponse, *schemas.BifrostError)) and addedSetEmbeddingRequestExecutoron the plugin to wire it to the global client at startup and on plugin reload.BootstrapandReloadPluginpaths now callSetEmbeddingRequestExecutor(s.Client.EmbeddingRequest)after the plugin is loaded.BifrostContextKeySkipPluginPipelineset totrue, preventing recursive plugin pipeline execution.AddProviderKeysToSemanticCacheConfig→ValidateSemanticCacheConfigto reflect that the function no longer injects keys — it only validates that the referenced provider exists in the global config.RemoveProviderKeysFromSemanticCacheConfigentirely, since keys are no longer stored in the plugin config.keysfield fromconfig.schema.jsonand the UI type system (CacheConfig,EditorCacheConfig,DirectCacheConfig,ProviderBackedCacheConfig, and Zod schemas).providerandembedding_model, notkeys).Keysfrom all config fixtures and adjustTestInvalidProviderRejectionto expectInitto succeed (provider validation now happens at request time via the global client).Type of change
Affected areas
How to test
config.jsonwithout specifyingkeysinside the semantic cache plugin config.config.json.keysinside the plugin config is silently ignored (field no longer exists).Breaking changes
The
Keysfield has been removed fromsemanticcache.Config. Any existingconfig.jsonfiles or Go code that setKeysinside the semantic cache plugin config must remove that field. Keys are now inherited automatically from the global provider configuration and no longer need to be specified.Security considerations
API keys are no longer duplicated into the plugin config or persisted to the config store as part of the plugin's configuration blob. This reduces the surface area for accidental key exposure in stored configs or API responses.
Checklist
docs/contributing/README.mdand followed the guidelines