semantic router #3: embedding executor - #5656
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesGovernance now accepts a concurrency-safe embedding executor, creates timeout-bounded internal requests, normalizes multiple embedding formats to Governance embedding execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant GovernancePlugin
participant EmbeddingRequest
participant BifrostEmbeddingResponse
Server->>GovernancePlugin: Wire s.Client.EmbeddingRequest
GovernancePlugin->>GovernancePlugin: Prepare timeout-bounded internal context
GovernancePlugin->>EmbeddingRequest: Send embedding request
EmbeddingRequest-->>GovernancePlugin: Return embedding response
GovernancePlugin->>GovernancePlugin: Normalize embedding values
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/governance/embedding.go (1)
68-75: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
generateEmbeddingdoesn't re-validateDimension >= 2likeCanClassifySemantically.If a caller ever invokes
generateEmbeddingwithout first gating onCanClassifySemantically, an under-specifiedDimension(e.g. 0 or 1) would slip through and produce a vector whose length is never checked against the configured dimension, risking silent mismatches downstream in similarity comparisons.🛡️ Optional defensive check
if semantic == nil || semantic.Provider == "" || semantic.EmbeddingModel == "" { return nil, 0, fmt.Errorf("semantic classification is not configured") } + if semantic.Dimension < 2 { + return nil, 0, fmt.Errorf("semantic classification dimension is not configured") + }🤖 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/governance/embedding.go` around lines 68 - 75, Update GovernancePlugin.generateEmbedding to validate semantic.Dimension is at least 2, matching CanClassifySemantically, before issuing the embedding request; return the existing configuration error path for invalid dimensions so direct callers cannot produce unchecked vector-length mismatches.
🤖 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.
Nitpick comments:
In `@plugins/governance/embedding.go`:
- Around line 68-75: Update GovernancePlugin.generateEmbedding to validate
semantic.Dimension is at least 2, matching CanClassifySemantically, before
issuing the embedding request; return the existing configuration error path for
invalid dimensions so direct callers cannot produce unchecked vector-length
mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d0bb6f7-10f9-4ebe-8e31-7c52b27420df
📒 Files selected for processing (4)
plugins/governance/embedding.goplugins/governance/embedding_test.goplugins/governance/main.gotransports/bifrost-http/server/server.go
00825bc to
aa0217c
Compare
44143a9 to
383a002
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/governance/embedding.go`:
- Line 97: Remove the BifrostContextKeySkipPluginPipeline assignment from the
embedding plugin and move the anti-recursion behavior into a core-owned executor
API or option. Update the relevant core execution path to apply this behavior
internally, then have the plugin invoke that API without setting reserved
context keys.
🪄 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 Plus
Run ID: 72d93fbc-300d-4ee7-b75c-102e93e9033c
📒 Files selected for processing (4)
plugins/governance/embedding.goplugins/governance/embedding_test.goplugins/governance/main.gotransports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/governance/main.go
aa0217c to
d2438a7
Compare
383a002 to
a7329f0
Compare
a7329f0 to
ae27dad
Compare

Summary
Wires the bifrost embedding client into the governance plugin so it can call out to an embedding provider for semantic complexity classification. Previously the governance plugin had no way to generate embeddings; this adds the executor interface, the embedding call logic, and the server-side wiring at both bootstrap and plugin reload time.
Changes
EmbeddingRequestExecutortype andEmbeddingExecutorSetterinterface to the governance plugin, mirroring the pattern already used by the semantic cache plugin.SetEmbeddingRequestExecutor,embeddingExecutor,CanClassifySemantically, andgenerateEmbeddingonGovernancePlugin. The executor is stored atomically to allow safe concurrent access during plugin reloads and request classification.generateEmbeddinghandles all embedding response encodings (string JSON, float64 array, 2D float64 array, int8, int32) by normalising them to[]float32for uniform cosine similarity comparison.SemanticConfig.Timeout(defaulting toconfigstore.DefaultComplexitySemanticTimeout), skips the plugin pipeline to prevent recursion, and clears caller routing state so the internal request behaves like a fresh external embeddings call.embeddingRequestExecutor atomic.Pointer[EmbeddingRequestExecutor]field toGovernancePlugin.EmbeddingExecutorSetterduring bothBootstrapandReloadPlugin, consistent with how the semantic cache plugin is handled.Type of change
Affected areas
How to test
Key test scenarios covered:
TestGenerateEmbeddingDecodesAllEncodings— verifies all five embedding wire formats are correctly normalised to[]float32.TestGenerateEmbeddingRequestShape— asserts the outbound request carries the correct provider, model, input text, skip-pipeline flag, and a bounded deadline.TestGenerateEmbeddingTimeoutCancelsCall— confirms a slow provider is cancelled within the configured timeout window.TestGenerateEmbeddingGuards— covers nil executor, nil semantic config, empty response, and executor teardown.TestCanClassifySemantically— validates the readiness gate across all partial-configuration combinations.Breaking changes
Related issues
Prerequisite for semantic complexity classification (task #5 — exemplar warmup).
Security considerations
The internal embedding request explicitly clears caller key-routing and body-transport state and skips the plugin pipeline, preventing the governance plugin from inadvertently inheriting or leaking caller credentials or routing context into the embedding call.
Checklist
docs/contributing/README.mdand followed the guidelines