Conversation
|
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRouting chain evaluation now tracks which routing rules have fired within a chain and skips re-matching the same rule, replacing prior provider/model-based cycle detection so chains continue until no candidate rules remain or max depth is reached. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Confidence Score: 5/5Safe to merge — targeted, well-reasoned bug fix with no regressions to existing termination guarantees No P0 or P1 issues found. The cycle-detection replacement is logically sound: each rule fires at most once, so infinite loops are still impossible, and chainMaxDepth provides a hard upper bound. Both the self-loop-continues and self-loop-alone cases are covered by new/updated tests. No files require special attention Important Files Changed
Reviews (4): Last reviewed commit: "fix: routing rule chain no longer halts ..." | 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}
Pratham-Mishra04
force-pushed
the
04-27-fix_routing_rule_chain_no_longer_halts_when_a_chain_rule_resolves_itself
branch
from
2dea414 to
11db74d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/governance/routing_test.go (1)
816-913: Add one non-self-loop cycle regression too.This change replaces state-cycle detection for all chains, not just self-loops. A two-rule oscillation like
A: gpt-4o -> gpt-4-turboandB: gpt-4-turbo -> gpt-4owould pin the broadervisitedRuleIDsbehavior and guard against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/routing_test.go` around lines 816 - 913, Add a new test (e.g., TestEvaluateRoutingRules_TwoRuleOscillationTerminates) that creates two distinct chain rules which map between each other's models (RuleA: model == 'gpt-4o' -> provider/openai model 'gpt-4-turbo'; RuleB: model == 'gpt-4-turbo' -> provider/openai model 'gpt-4o'), register them with store.UpdateRoutingRuleInMemory, set the initial RoutingContext to Provider=schemas.OpenAI and Model="gpt-4o", call engine.EvaluateRoutingRules(bgCtx, ctx) and assert it returns successfully (no infinite loop) and produces a finite Decision (non-nil) — verifying the routing engine's visitedRuleIDs cycle-detection stops the oscillation after the rules fire once each; use NewLocalGovernanceStore, NewRoutingEngine and EvaluateRoutingRules to locate relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/governance/routing_test.go`:
- Around line 816-913: Add a new test (e.g.,
TestEvaluateRoutingRules_TwoRuleOscillationTerminates) that creates two distinct
chain rules which map between each other's models (RuleA: model == 'gpt-4o' ->
provider/openai model 'gpt-4-turbo'; RuleB: model == 'gpt-4-turbo' ->
provider/openai model 'gpt-4o'), register them with
store.UpdateRoutingRuleInMemory, set the initial RoutingContext to
Provider=schemas.OpenAI and Model="gpt-4o", call
engine.EvaluateRoutingRules(bgCtx, ctx) and assert it returns successfully (no
infinite loop) and produces a finite Decision (non-nil) — verifying the routing
engine's visitedRuleIDs cycle-detection stops the oscillation after the rules
fire once each; use NewLocalGovernanceStore, NewRoutingEngine and
EvaluateRoutingRules to locate relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6435ba0d-c91d-4ab7-beeb-a2fb1841e180
📒 Files selected for processing (4)
plugins/governance/changelog.mdplugins/governance/routing.goplugins/governance/routing_test.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (2)
- transports/changelog.md
- plugins/governance/changelog.md
Merge activity
|
Pratham-Mishra04
force-pushed
the
04-27-refactor_use_global_bifrost_client_in_semmantic_cache_plugin
branch
from
7fc9839 to
92ddedd
Compare
Pratham-Mishra04
force-pushed
the
04-27-fix_routing_rule_chain_no_longer_halts_when_a_chain_rule_resolves_itself
branch
from
11db74d to
201f9f6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/governance/routing.go (1)
157-161: Consider including rule ID in skip logs for disambiguation.This logic is correct; optional tweak: add
rule.IDto skip logs so duplicate names across scopes are easier to trace.🔧 Optional logging improvement
- re.logger.Debug("[RoutingEngine] Skipping rule %s (already fired this chain)", rule.Name) - ctx.AppendRoutingEngineLog(schemas.RoutingEngineRoutingRule, schemas.LogLevelInfo, fmt.Sprintf("Rule '%s' skipped: already fired in this chain", rule.Name)) + re.logger.Debug("[RoutingEngine] Skipping rule %s (id=%s) (already fired this chain)", rule.Name, rule.ID) + ctx.AppendRoutingEngineLog( + schemas.RoutingEngineRoutingRule, + schemas.LogLevelInfo, + fmt.Sprintf("Rule '%s' (id=%s) skipped: already fired in this chain", rule.Name, rule.ID), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/routing.go` around lines 157 - 161, Add rule.ID to the skip log statements to disambiguate rules with duplicate names: update the visitedRuleIDs check branch in the routing loop (where visitedRuleIDs, rule.ID, rule.Name are used) so re.logger.Debug includes rule.ID and ctx.AppendRoutingEngineLog (schemas.RoutingEngineRoutingRule, schemas.LogLevelInfo) also logs the ID alongside the rule name in its formatted message; keep existing messages and continue behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/governance/routing.go`:
- Around line 157-161: Add rule.ID to the skip log statements to disambiguate
rules with duplicate names: update the visitedRuleIDs check branch in the
routing loop (where visitedRuleIDs, rule.ID, rule.Name are used) so
re.logger.Debug includes rule.ID and ctx.AppendRoutingEngineLog
(schemas.RoutingEngineRoutingRule, schemas.LogLevelInfo) also logs the ID
alongside the rule name in its formatted message; keep existing messages and
continue behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 168c6b90-b3d8-4aea-a05c-817b4bb061b5
📒 Files selected for processing (4)
plugins/governance/changelog.mdplugins/governance/routing.goplugins/governance/routing_test.gotransports/changelog.md
✅ Files skipped from review due to trivial changes (1)
- plugins/governance/changelog.md
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/changelog.md
akshaydeo
changed the base branch from
04-27-refactor_use_global_bifrost_client_in_semmantic_cache_plugin
to
graphite-base/3082
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
akshaydeo
force-pushed
the
04-27-fix_routing_rule_chain_no_longer_halts_when_a_chain_rule_resolves_itself
branch
from
201f9f6 to
abbda48
Compare
akshaydeo
deleted the
04-27-fix_routing_rule_chain_no_longer_halts_when_a_chain_rule_resolves_itself
branch
3 participants
Summary
Fixes a bug where a routing rule chain would incorrectly halt when a
chain_rule=truerule resolved to the same provider/model as the current context (a self-loop). Previously, this was treated as a cycle and stopped chain evaluation immediately. Now, each rule is tracked by its ID rather than by the resulting provider/model state, allowing a self-looping rule to fire once and then be skipped so subsequent rules in the chain can continue to evaluate.Changes
visitedmap keyed onprovider|model) with rule ID-based tracking (visitedRuleIDsmap keyed onrule.ID). Each rule may now fire at most once per chain evaluation, regardless of whether its target matches the current state.EvaluateRoutingRulesdocumentation to reflect that the chain stops when all candidate chain-rules have already fired, rather than when a previously visited provider/model state is encountered.TestEvaluateRoutingRules_ConvergenceStopsChaintoTestEvaluateRoutingRules_SelfLoopContinuesToNextRuleand updated it to assert that a self-looping rule correctly yields to the next matching rule.TestEvaluateRoutingRules_SelfLoopAloneTerminatesto verify that a self-looping chain rule with no other rules terminates cleanly via the "no remaining rule matches" condition.Type of change
Affected areas
How to test
Expected:
TestEvaluateRoutingRules_SelfLoopContinuesToNextRulepasses, asserting the chain resolves toanthropic/claude-3after the self-looping rule fires once.TestEvaluateRoutingRules_SelfLoopAloneTerminatespasses, asserting the chain resolves toopenai/gpt-4oand terminates cleanly.Breaking changes
Related issues
Security considerations
None.
Checklist
docs/contributing/README.mdand followed the guidelines