fix: merge agenticTiers/ecoTiers/premiumTiers in routing config overrides#150
fix: merge agenticTiers/ecoTiers/premiumTiers in routing config overrides#150kagura-agent wants to merge 3 commits intoBlockRunAI:mainfrom
Conversation
…ides mergeRoutingConfig only deep-merged tiers, classifier, scoring, and overrides from user config. agenticTiers, ecoTiers, and premiumTiers were silently ignored, making it impossible to customize which models handle agentic/eco/premium requests. Now all three tier sets are properly merged with three-case handling: - Not provided → keeps default - Explicitly null → disables that tier set - Object → deep-merges with default Fixes BlockRunAI#148
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/proxy.ts`:
- Around line 1288-1296: mergeTierRecord currently shallow-merges the top-level
mapping so a tier's override replaces the entire TierConfig object (losing
default fields); update mergeTierRecord to perform a per-tier merge: iterate
over all tiers present in base or override and for each tier produce
mergedConfig = { ...base[tier], ...override[tier] } (respecting the early-return
rules: override === null => undefined, override === undefined => base, if !base
return override) so partial TierConfig overrides preserve unspecified default
fields; reference function mergeTierRecord and type TierConfig to locate where
to change the merge logic.
- Around line 1298-1314: Change the mergeRoutingConfig signature to accept a new
specialized override type that permits null for the three tier records: create a
RoutingConfigOverrides type (based on Partial<RoutingConfig>) but with
agenticTiers, ecoTiers, and premiumTiers typed as Record<Tier, TierConfig> |
null | undefined; then update mergeRoutingConfig(overrides?:
RoutingConfigOverrides) and keep the same runtime merge logic using
mergeTierRecord for agenticTiers/ecoTiers/premiumTiers so callers/tests can pass
null without casting. Ensure references to RoutingConfig, mergeRoutingConfig,
agenticTiers, ecoTiers, and premiumTiers are updated to the new type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b84d49e9-99e2-46a7-af20-6c8ca679e38b
📒 Files selected for processing (3)
src/proxy.merge-routing.test.tssrc/proxy.tssrc/router/index.ts
| export function mergeTierRecord( | ||
| base: Record<Tier, TierConfig> | undefined, | ||
| override: Record<Tier, TierConfig> | null | undefined, | ||
| ): Record<Tier, TierConfig> | undefined { | ||
| if (override === null) return undefined; // explicitly disabled | ||
| if (override === undefined) return base; // not provided, keep default | ||
| if (!base) return override; // no default, use override as-is | ||
| return { ...base, ...override }; | ||
| } |
There was a problem hiding this comment.
mergeTierRecord is not deep-merging per-tier configs.
At Line 1295, { ...base, ...override } replaces the whole tier entry. If an override provides only part of a TierConfig, default fields in that tier are lost instead of merged.
Proposed fix
export function mergeTierRecord(
base: Record<Tier, TierConfig> | undefined,
override: Record<Tier, TierConfig> | null | undefined,
): Record<Tier, TierConfig> | undefined {
if (override === null) return undefined; // explicitly disabled
if (override === undefined) return base; // not provided, keep default
if (!base) return override; // no default, use override as-is
- return { ...base, ...override };
+ const merged = { ...base };
+ for (const tier of Object.keys(override) as Tier[]) {
+ merged[tier] = { ...base[tier], ...override[tier] };
+ }
+ return merged;
}📝 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.
| export function mergeTierRecord( | |
| base: Record<Tier, TierConfig> | undefined, | |
| override: Record<Tier, TierConfig> | null | undefined, | |
| ): Record<Tier, TierConfig> | undefined { | |
| if (override === null) return undefined; // explicitly disabled | |
| if (override === undefined) return base; // not provided, keep default | |
| if (!base) return override; // no default, use override as-is | |
| return { ...base, ...override }; | |
| } | |
| export function mergeTierRecord( | |
| base: Record<Tier, TierConfig> | undefined, | |
| override: Record<Tier, TierConfig> | null | undefined, | |
| ): Record<Tier, TierConfig> | undefined { | |
| if (override === null) return undefined; // explicitly disabled | |
| if (override === undefined) return base; // not provided, keep default | |
| if (!base) return override; // no default, use override as-is | |
| const merged = { ...base }; | |
| for (const tier of Object.keys(override) as Tier[]) { | |
| merged[tier] = { ...base[tier], ...override[tier] }; | |
| } | |
| return merged; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/proxy.ts` around lines 1288 - 1296, mergeTierRecord currently
shallow-merges the top-level mapping so a tier's override replaces the entire
TierConfig object (losing default fields); update mergeTierRecord to perform a
per-tier merge: iterate over all tiers present in base or override and for each
tier produce mergedConfig = { ...base[tier], ...override[tier] } (respecting the
early-return rules: override === null => undefined, override === undefined =>
base, if !base return override) so partial TierConfig overrides preserve
unspecified default fields; reference function mergeTierRecord and type
TierConfig to locate where to change the merge logic.
src/proxy.ts
Outdated
| export function mergeRoutingConfig(overrides?: Partial<RoutingConfig>): RoutingConfig { | ||
| if (!overrides) return DEFAULT_ROUTING_CONFIG; | ||
| return { | ||
| ...DEFAULT_ROUTING_CONFIG, | ||
| ...overrides, | ||
| classifier: { ...DEFAULT_ROUTING_CONFIG.classifier, ...overrides.classifier }, | ||
| scoring: { ...DEFAULT_ROUTING_CONFIG.scoring, ...overrides.scoring }, | ||
| tiers: { ...DEFAULT_ROUTING_CONFIG.tiers, ...overrides.tiers }, | ||
| agenticTiers: mergeTierRecord( | ||
| DEFAULT_ROUTING_CONFIG.agenticTiers, | ||
| overrides.agenticTiers, | ||
| ), | ||
| ecoTiers: mergeTierRecord(DEFAULT_ROUTING_CONFIG.ecoTiers, overrides.ecoTiers), | ||
| premiumTiers: mergeTierRecord( | ||
| DEFAULT_ROUTING_CONFIG.premiumTiers, | ||
| overrides.premiumTiers, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current mergeRoutingConfig signature
rg -nP 'export function mergeRoutingConfig\(overrides\?: Partial<RoutingConfig>\)' src/proxy.ts
# Verify tests currently require null casts to bypass typing
rg -nP 'null as unknown as Record<Tier, TierConfig>' src/proxy.merge-routing.test.tsRepository: BlockRunAI/ClawRouter
Length of output: 427
Fix mergeRoutingConfig type signature to allow null for tier properties.
The function signature on line 1298 uses Partial<RoutingConfig>, which does not permit null values for agenticTiers, ecoTiers, and premiumTiers, even though the runtime implementation supports them. This forces tests to use type casts (null as unknown as Record<Tier, TierConfig>) to pass null values.
Create a specialized RoutingConfigOverrides type that allows null for these three properties:
Proposed fix
+type RoutingConfigOverrides = Omit<
+ Partial<RoutingConfig>,
+ "agenticTiers" | "ecoTiers" | "premiumTiers"
+> & {
+ agenticTiers?: Record<Tier, TierConfig> | null;
+ ecoTiers?: Record<Tier, TierConfig> | null;
+ premiumTiers?: Record<Tier, TierConfig> | null;
+};
+
-export function mergeRoutingConfig(overrides?: Partial<RoutingConfig>): RoutingConfig {
+export function mergeRoutingConfig(overrides?: RoutingConfigOverrides): RoutingConfig {📝 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.
| export function mergeRoutingConfig(overrides?: Partial<RoutingConfig>): RoutingConfig { | |
| if (!overrides) return DEFAULT_ROUTING_CONFIG; | |
| return { | |
| ...DEFAULT_ROUTING_CONFIG, | |
| ...overrides, | |
| classifier: { ...DEFAULT_ROUTING_CONFIG.classifier, ...overrides.classifier }, | |
| scoring: { ...DEFAULT_ROUTING_CONFIG.scoring, ...overrides.scoring }, | |
| tiers: { ...DEFAULT_ROUTING_CONFIG.tiers, ...overrides.tiers }, | |
| agenticTiers: mergeTierRecord( | |
| DEFAULT_ROUTING_CONFIG.agenticTiers, | |
| overrides.agenticTiers, | |
| ), | |
| ecoTiers: mergeTierRecord(DEFAULT_ROUTING_CONFIG.ecoTiers, overrides.ecoTiers), | |
| premiumTiers: mergeTierRecord( | |
| DEFAULT_ROUTING_CONFIG.premiumTiers, | |
| overrides.premiumTiers, | |
| ), | |
| type RoutingConfigOverrides = Omit< | |
| Partial<RoutingConfig>, | |
| "agenticTiers" | "ecoTiers" | "premiumTiers" | |
| > & { | |
| agenticTiers?: Record<Tier, TierConfig> | null; | |
| ecoTiers?: Record<Tier, TierConfig> | null; | |
| premiumTiers?: Record<Tier, TierConfig> | null; | |
| }; | |
| export function mergeRoutingConfig(overrides?: RoutingConfigOverrides): RoutingConfig { | |
| if (!overrides) return DEFAULT_ROUTING_CONFIG; | |
| return { | |
| ...DEFAULT_ROUTING_CONFIG, | |
| ...overrides, | |
| classifier: { ...DEFAULT_ROUTING_CONFIG.classifier, ...overrides.classifier }, | |
| scoring: { ...DEFAULT_ROUTING_CONFIG.scoring, ...overrides.scoring }, | |
| tiers: { ...DEFAULT_ROUTING_CONFIG.tiers, ...overrides.tiers }, | |
| agenticTiers: mergeTierRecord( | |
| DEFAULT_ROUTING_CONFIG.agenticTiers, | |
| overrides.agenticTiers, | |
| ), | |
| ecoTiers: mergeTierRecord(DEFAULT_ROUTING_CONFIG.ecoTiers, overrides.ecoTiers), | |
| premiumTiers: mergeTierRecord( | |
| DEFAULT_ROUTING_CONFIG.premiumTiers, | |
| overrides.premiumTiers, | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/proxy.ts` around lines 1298 - 1314, Change the mergeRoutingConfig
signature to accept a new specialized override type that permits null for the
three tier records: create a RoutingConfigOverrides type (based on
Partial<RoutingConfig>) but with agenticTiers, ecoTiers, and premiumTiers typed
as Record<Tier, TierConfig> | null | undefined; then update
mergeRoutingConfig(overrides?: RoutingConfigOverrides) and keep the same runtime
merge logic using mergeTierRecord for agenticTiers/ecoTiers/premiumTiers so
callers/tests can pass null without casting. Ensure references to RoutingConfig,
mergeRoutingConfig, agenticTiers, ecoTiers, and premiumTiers are updated to the
new type.
|
Same situation as #149 — this fixes part of #148, but #148 has two coupled bugs and v0.12.141 (shipped a few hours ago) fixes both. This PR only addresses the first. Both bugs from #148
This PR fixes (1). v0.12.141 also fixes (2): // src/router/strategy.ts (post v0.12.141)
let useAgenticTiers: boolean;
if (agenticModeSetting === false) {
useAgenticTiers = false; // explicitly disabled
} else if (agenticModeSetting === true) {
useAgenticTiers = config.agenticTiers != null; // explicitly forced
} else {
useAgenticTiers = (hasToolsInRequest || isAutoAgentic) && config.agenticTiers != null;
}Without (2), even after merging your custom `agenticTiers` correctly, `agenticMode: false` still wouldn't actually let users opt out, because `hasToolsInRequest` would always force agentic mode for agent workloads. What's nice about this PR
Suggested path forwardEither:
Up to you — the cleanup is genuinely worth landing, just want to avoid churn now that the user-facing bug is fixed. cc @jeegankones who reported the issue. |
|
Great analysis @1bcMax — makes total sense that this only fixes half of #148 without the I'll go with (b): rebase onto main, drop the duplicate |
|
Closing — v0.12.141+ already includes a comprehensive fix for the tier merging, including the |
2 participants
Problem
mergeRoutingConfigonly deep-mergestiers,classifier,scoring, andoverridesfrom user config. The three specialized tier sets (agenticTiers,ecoTiers,premiumTiers) are silently ignored, making it impossible to customize which models handle agentic/eco/premium requests.Since every agent request includes tools (
hasToolsInRequest = true),agenticTiersis always used for agent workloads. Customtiersdefinitions have no effect on actual agent traffic.Fix
Added
mergeTierRecord()helper with three-case handling:undefined) → keeps defaultnull→ disables that tier set (sets toundefined)Applied to all three optional tier sets in
mergeRoutingConfig().Tests
13 new tests covering:
mergeTierRecordbase cases (null, undefined, merge, no-default)agenticTiersoverride merges correctlyagenticTiers: nulldisables agentic tier selectionecoTiers/premiumTierscustom overridesAll 409 tests pass. TypeCheck clean.
Fixes #148
Summary by CodeRabbit
Tests
Chores