updated docs + helm changes - #5924
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR documents semantic embedding classification, reference phrases, classifier status, OpenAPI contracts, Helm configuration, legacy compatibility, validation, and observability changes. It also documents Chromem vector-store support. ChangesSemantic Complexity Routing
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR moves complexity routing to semantic embeddings and updates its configuration schema. The current schema still permits a zero timeout and describes one legacy field as ignored even though it is retained for deprecation reporting, which could allow invalid configuration or confuse operators; the PR is otherwise mergeable with explicit owner follow-up. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/openapi/openapi.json (1)
55622-55657: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
keywordsrejects the legacy four-list shape.
transports/config.schema.jsonaccepts the old four-list reference-phrase shape for compatibility, and treatscomplex_reasoningas deprecated and ignored. This schema setsadditionalProperties: falseonkeywordsand lists only the three tier arrays. A legacy payload that still carriescomplex_reasoning_keywordstherefore fails documented validation while the transport still accepts it.Add the deprecated fourth list as an optional, deprecated property, or relax
additionalProperties. Apply the same change to the PUT request copy (Line 55783), the PUT response copy (Line 55916), and the reset response copy (Line 56090).As per path instructions, "Treat transports/config.schema.json as the source of truth for config fields", and the referenced schema states that "the old four-list shape remains accepted" while "legacy complex_reasoning is deprecated and ignored".
🔧 Proposed fix for the GET response copy
"properties": { "simple_keywords": { "type": "array", "description": "Reference phrases for the SIMPLE tier (50 built-in defaults)", "items": { "type": "string" }, "minItems": 1 }, + "complex_reasoning_keywords": { + "type": "array", + "deprecated": true, + "description": "Deprecated legacy fourth reference-phrase list. Still accepted so older configurations load, but ignored by semantic classification.", + "items": { + "type": "string" + } + },🤖 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 `@docs/openapi/openapi.json` around lines 55622 - 55657, Update each OpenAPI keywords schema copy in the GET response, PUT request, PUT response, and reset response to accept the legacy optional complex_reasoning_keywords array, mark it deprecated, and keep additionalProperties false. Match the property’s type and compatibility behavior from transports/config.schema.json, while preserving the existing three required tier arrays and treating the legacy field as ignored.Sources: Coding guidelines, Path instructions
docs/openapi/schemas/management/governance.yaml (1)
2009-2107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the OpenAPI configuration contract with
transports/config.schema.json.
ComplexityTierBoundariesrejects legacycomplex_reasoning.ComplexityKeywordConfigrejects the accepted legacy four-list shape. The transport schema accepts both for migration compatibility.The semantic fields also omit source constraints. This schema accepts empty provider values,
dimension: 1, negative timeouts, invalid similarity values, and invalid history counts. It also documents1.5s, but the source schema accepts whole-number duration strings such as1500ms.Mirror the legacy branches, validation bounds, duration pattern, and
"1500ms"default fromtransports/config.schema.json. This prevents OpenAPI clients from producing configurations that source validation rejects. As per coding guidelines,transports/config.schema.jsonis the source of truth for configuration fields. As per path instructions, docs must matchconfig.schema.jsonand provider behavior.🤖 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 `@docs/openapi/schemas/management/governance.yaml` around lines 2009 - 2107, Align ComplexityTierBoundaries and ComplexityKeywordConfig with the corresponding legacy-compatible branches in transports/config.schema.json, including complex_reasoning and the accepted four-list keyword shape. Update ComplexitySemanticConfig to mirror source validation constraints for provider, dimension, timeout, min_similarity, and message_history_count, including the whole-number duration pattern and 1500ms default. Treat transports/config.schema.json as the authoritative contract and preserve its provider-related constraints.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (2)
docs/providers/routing-rules.mdx (1)
127-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd embedding failure and timeout to the unavailable-classification list.
docs/features/governance/complexity-router.mdx(Line 366) lists embedding failure and timeout as skip causes, andtransports/config.schema.jsontreats a timeout as a skipped classification. This note omits both.📝 Proposed wording
-If complexity classification is unavailable for a request (classifier not configured or not ready, unsupported input, or a match below the similarity floor), `complexity_tier` is treated as **unknown** by the evaluator — the rule does not match and evaluation falls through. Rules using other variables are unaffected. +If complexity classification is unavailable for a request (classifier not configured or not ready, unsupported input, an embedding failure or timeout, or a match below the similarity floor), `complexity_tier` is treated as **unknown** by the evaluator — the rule does not match and evaluation falls through. Rules using other variables are unaffected.🤖 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 `@docs/providers/routing-rules.mdx` around lines 127 - 129, Update the Note describing unavailable complexity classification to include embedding failures and timeouts alongside the existing classifier, unsupported-input, and similarity-floor cases, while preserving the documented unknown-tier fallthrough behavior.Source: Coding guidelines
docs/openapi/openapi.json (1)
56209-56253: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd error responses to the status endpoint.
The sibling governance endpoints in this file define
500and503responses that referenceBifrostError.getComplexityAnalyzerStatusdefines only200. Clients cannot see the documented failure contract. Add the same500and503responses for parity.♻️ Proposed addition
"error": { "type": "string", "description": "Warmup failure detail when state=failed" } } } } } - } + }, + "500": { + "description": "Internal server error", + "content": { + "application/json": { + "schema": { + "$ref": "`#/components/schemas/BifrostError`" + } + } + } + }, + "503": { + "description": "Config store not available", + "content": { + "application/json": { + "schema": { + "$ref": "`#/components/schemas/BifrostError`" + } + } + } + }🤖 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 `@docs/openapi/openapi.json` around lines 56209 - 56253, Update the getComplexityAnalyzerStatus endpoint responses to include the same documented 500 and 503 error responses as its sibling governance endpoints, referencing the existing BifrostError schema. Preserve the current 200 response and status schema unchanged.
🤖 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 `@docs/deployment-guides/helm/governance.mdx`:
- Around line 397-417: Update the complexityAnalyzerConfig semantic timeout
value in the governance deployment example from "1.5s" to the schema-valid
"1500ms" format.
In `@docs/features/governance/complexity-router.mdx`:
- Line 312: Update the complexity_mechanism documentation in the table and
logs-filter guidance to include the historical lexical value, indicating that it
remains filterable for pre-upgrade data. Mirror the existing legacy-value
handling documented for REASONING without changing the current semantic and
skipped definitions.
- Line 52: Align the configuration schema with the validation described in
complexity-router.mdx: update transports/config.schema.json to enforce at most
2000 characters per phrase and prevent normalized duplicate phrases across
tiers, or explicitly document that framework/configstore/complexityconfig.go
applies these constraints during save. Ensure the schema or documentation also
reflects the requirement that every tier contains at least one phrase.
In `@docs/openapi/openapi.json`:
- Around line 55676-55679: Update the four inlined semantic.dimension schemas in
docs/openapi/openapi.json at lines 55676-55679, 55837-55839, 55970-55972, and
56144-56146 to include a minimum constraint of 2 alongside the existing integer
type and description, matching transports/config.schema.json.
In `@helm-charts/bifrost/values.schema.json`:
- Around line 2071-2082: Update the timeout string pattern in the schema’s
timeout definition to accept documented fractional duration values such as 1.5s
while retaining the supported units and existing numeric timeout validation.
Keep transports/config.schema.json and the Helm schema/documentation behavior in
parity.
---
Outside diff comments:
In `@docs/openapi/openapi.json`:
- Around line 55622-55657: Update each OpenAPI keywords schema copy in the GET
response, PUT request, PUT response, and reset response to accept the legacy
optional complex_reasoning_keywords array, mark it deprecated, and keep
additionalProperties false. Match the property’s type and compatibility behavior
from transports/config.schema.json, while preserving the existing three required
tier arrays and treating the legacy field as ignored.
In `@docs/openapi/schemas/management/governance.yaml`:
- Around line 2009-2107: Align ComplexityTierBoundaries and
ComplexityKeywordConfig with the corresponding legacy-compatible branches in
transports/config.schema.json, including complex_reasoning and the accepted
four-list keyword shape. Update ComplexitySemanticConfig to mirror source
validation constraints for provider, dimension, timeout, min_similarity, and
message_history_count, including the whole-number duration pattern and 1500ms
default. Treat transports/config.schema.json as the authoritative contract and
preserve its provider-related constraints.
---
Nitpick comments:
In `@docs/openapi/openapi.json`:
- Around line 56209-56253: Update the getComplexityAnalyzerStatus endpoint
responses to include the same documented 500 and 503 error responses as its
sibling governance endpoints, referencing the existing BifrostError schema.
Preserve the current 200 response and status schema unchanged.
In `@docs/providers/routing-rules.mdx`:
- Around line 127-129: Update the Note describing unavailable complexity
classification to include embedding failures and timeouts alongside the existing
classifier, unsupported-input, and similarity-floor cases, while preserving the
documented unknown-tier fallthrough behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39ff6b5d-3953-4545-8bf1-814ed3786291
⛔ Files ignored due to path filters (8)
docs/media/architecture-complexity-router.pngis excluded by!**/*.pngdocs/media/complexity-logic-architecture.pngis excluded by!**/*.pngdocs/media/ui-complexity-router-config.pngis excluded by!**/*.pngdocs/media/ui-complexity-router-embedding-configuration.pngis excluded by!**/*.pngdocs/media/ui-complexity-router-keywords.pngis excluded by!**/*.pngdocs/media/ui-complexity-router-semantic.pngis excluded by!**/*.pngdocs/media/ui-routing-logs-complexity.pngis excluded by!**/*.pngdocs/media/ui-routing-rule-complexity.pngis excluded by!**/*.png
📒 Files selected for processing (16)
docs/deployment-guides/helm/governance.mdxdocs/features/governance/complexity-router.mdxdocs/features/observability/prometheus.mdxdocs/features/telemetry.mdxdocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/governance.yamldocs/openapi/paths/management/logging.yamldocs/openapi/schemas/management/governance.yamldocs/openapi/schemas/management/logging.yamldocs/providers/routing-rules.mdxhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamltransports/config.schema.json
e23bb9d to
dbd33bd
Compare
14c8e26 to
c0528e5
Compare
b309e64 to
1132530
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
c0528e5 to
bbfe14b
Compare
1132530 to
070dfec
Compare
070dfec to
4f74cee
Compare
1413129 to
70ef74f
Compare
4f74cee to
262bbf3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm-charts/bifrost/values.schema.json (1)
2107-2184: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the default claim with schema validation.
DefaultAnalyzerConfig()supplies 50 phrases only when the entire configuration is absent. Both schemas requirekeywordsfor any non-null configuration, and Helm forwards the supplied object unchanged. A semantic-only configuration therefore fails validation. Remove the default claim or makekeywordsoptional and apply defaults before validation.🤖 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 `@helm-charts/bifrost/values.schema.json` around lines 2107 - 2184, Update the `keywords` schema definition to match `DefaultAnalyzerConfig()` behavior: configurations that provide other fields without `keywords` must validate successfully because defaults are only applied when the entire configuration is absent. Remove the schema’s required `keywords` constraint while preserving validation for supplied keyword objects and both supported shapes.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@helm-charts/bifrost/values.schema.json`:
- Around line 2107-2184: Update the `keywords` schema definition to match
`DefaultAnalyzerConfig()` behavior: configurations that provide other fields
without `keywords` must validate successfully because defaults are only applied
when the entire configuration is absent. Remove the schema’s required `keywords`
constraint while preserving validation for supplied keyword objects and both
supported shapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0ec2f4e-08c4-4584-bb1e-ec9d432dc83e
📒 Files selected for processing (8)
docs/deployment-guides/helm/governance.mdxdocs/features/governance/complexity-router.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/governance.yamlhelm-charts/bifrost/README.mdhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamltransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (4)
- helm-charts/bifrost/README.md
- docs/deployment-guides/helm/governance.mdx
- docs/openapi/schemas/management/governance.yaml
- docs/openapi/openapi.json
a9a1ebe to
305ece4
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 `@transports/config.schema.json`:
- Around line 3614-3621: Update the complex_reasoning schema description to
limit the “ignored” claim to semantic classification, while stating that
compatibility loading may retain or record the field’s presence for
backward-compatible handling. Preserve the existing deprecation and
legacy-configuration compatibility context.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9435b76b-2f52-4ba1-91b4-414b0d8353f4
📒 Files selected for processing (8)
docs/deployment-guides/helm/governance.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/governance.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamltransports/bifrost-http/handlers/governance_test.gotransports/config.schema.jsontransports/schema_test/config_schema_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/openapi/schemas/management/governance.yaml
- transports/bifrost-http/handlers/governance_test.go
- helm-charts/bifrost/values.yaml
- docs/deployment-guides/helm/governance.mdx
- helm-charts/bifrost/values.schema.json
- docs/openapi/openapi.json
70ef74f to
c365ed1
Compare
305ece4 to
9d2c671
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/config.schema.json (1)
3723-3735: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject zero-valued string timeouts.
The pattern accepts
0ns,0ms, and0.0s, while the numeric form rejects0. Runtime normalization falls back to the default for zero, but the schema still permits a value prohibited by the positive-timeout contract.Reject zero-valued strings and add schema cases for
0ms,0.0s, and0.5s.🤖 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/config.schema.json` around lines 3723 - 3735, Update the timeout schema’s string pattern to reject zero-valued duration strings while continuing to accept positive values such as 0.5s. Add schema cases covering 0ms and 0.0s as rejected inputs and 0.5s as an accepted input, alongside the existing numeric positivity validation.Source: Path instructions
🤖 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.
Outside diff comments:
In `@transports/config.schema.json`:
- Around line 3723-3735: Update the timeout schema’s string pattern to reject
zero-valued duration strings while continuing to accept positive values such as
0.5s. Add schema cases covering 0ms and 0.0s as rejected inputs and 0.5s as an
accepted input, alongside the existing numeric positivity validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a579222d-9936-4885-b122-695fabf9023c
📒 Files selected for processing (1)
transports/config.schema.json
c365ed1 to
b538bf9
Compare
9d2c671 to
e246f5f
Compare
2 participants
Summary
Replaces the Complexity Router's lexical keyword scorer with a semantic (embedding-based) classifier. Instead of matching weighted keyword lists, Bifrost now embeds each incoming request and assigns it the tier of its nearest reference phrase. The lexical scorer is fully retired; existing configurations continue to load but no tier is published until an embedding provider and model are configured.
Changes
message_history_countuser messages) and finds the nearest reference phrase across Simple, Medium, and Complex tiers. Numerictier_boundaries, conversation blending, and Complex keyword overrides no longer apply.semanticconfiguration block. Addsprovider,embedding_model,dimension,timeout(default 1.5s),min_similarity,message_history_count,count_toward_budgets, andvector_store(embeddedorvector_store) tocomplexity_analyzer_config. Without this block,complexity_tieris never published and complexity rules fall through.simple_keywords,medium_keywords, andcomplex_keywordsfields now hold whole example prompts rather than individual scoring keywords. 150 built-in defaults (50 per tier) are shipped, balanced across use cases and writing styles.GET /api/governance/complexity-analyzer-statusendpoint reportsdisabled,warming,ready, orfailed, with progress counters and aserving_previousflag so the last good generation keeps routing while a new one warms.complexity_mechanismupdated. The value is nowsemanticfor new requests;lexicalappears only on historical log rows.complexity_scorenow reflects the nearest-phrase similarity rather than a weighted keyword score.bifrost_routing_embedding_requests_totalandbifrost_routing_embedding_cost_totaltrack embedding overhead from semantic routing, labeled by provider, model, and phase (requestvswarmup).complexityAnalyzerConfiggains thesemanticblock with full schema validation.tier_boundariesis retained for backward compatibility but marked legacy.complex_reasoningboundary is accepted but ignored.vectorStore.typenow acceptschromem, and achromemsection is added tovalues.yamland_helpers.tpl.complex_reasoningboundaries parse without error. User-added entries are mapped from four tiers to three (Code + Technical → Medium, Reasoning → Complex). No tier is published until thesemanticblock is added.ComplexitySemanticConfigandComplexitySemanticStatusschemas are added. Allcomplexity_mechanismsfilter descriptions updated.resetComplexityAnalyzerConfignow preserves the embedding configuration.config.schema.jsonremoves thefallbackfield and updates the default timeout to1500ms.Type of change
Affected areas
How to test
go test ./...complexity_analyzer_configwith asemanticblock pointing to a valid embedding provider and model.GET /api/governance/complexity-analyzer-status— wait forstate: ready.complexity_tier. Confirm the routing decision log recordscomplexity_mechanism: semantic, a similarity score, and the matched reference phrase.min_similarityabove the returned score and confirm the tier is no longer published (complexity_mechanism: skipped).semanticblock and confirmstate: disabledand no tier is published.code_keywords/technical_keywords/reasoning_keywordsand confirm startup succeeds without error.bifrost_routing_embedding_requests_totalandbifrost_routing_embedding_cost_totalincrement in Prometheus after classified requests.Breaking changes
The lexical scorer no longer runs. Deployments that relied on keyword-based classification will publish no tier (
complexity_mechanism: skipped) until asemanticblock is added and an embedding provider is configured. Existing configurations load without error, so startup is safe. Any routing rules matching on"REASONING"should be updated to"COMPLEX". Custom keyword entries are preserved and mapped to the nearest tier but should be replaced with complete example phrases for accurate semantic matching.Related issues
Security considerations
The classifier status endpoint (
GET /api/governance/complexity-analyzer-status) never returns phrases, embeddings, or provider secrets. Embedding calls are bounded by a configurable timeout (default 1.5s) to prevent inline request-path blocking. Thecount_toward_budgetsflag controls whether embedding usage is recorded against virtual-key budgets but is never enforced as a hard limit.Checklist
docs/contributing/README.mdand followed the guidelines