feat(router): make portfolio routing the Auto default - #238
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe router now uses the V3 portfolio strategy by default. It classifies requests, ranks capable models with pricing and performance data, supports tool inference and capacity-safe fallbacks, and exposes local shadow-routing comparisons. ChangesPortfolio routing and proxy integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant route
participant PortfolioStrategy
participant UpstreamModel
participant onShadowRouted
Client->>Proxy: submit request with tools and output requirements
Proxy->>route: provide routing signals
route->>PortfolioStrategy: classify and rank candidates
PortfolioStrategy-->>route: return selected model and fallbacks
route-->>Proxy: return serving decision
Proxy->>UpstreamModel: send one completion request
Proxy->>route: evaluate sampled shadow strategy locally
Proxy->>onShadowRouted: report routing comparison
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 10
🧹 Nitpick comments (7)
scripts/smoke-dist.mjs (1)
70-78: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate the public
./routerexport.This check imports
dist/router/index.jsdirectly. It bypassespackage.json's./routerexport and does not verifydist/router/index.d.ts. A broken export map or declaration path can pass this smoke test. Import@blockrun/clawrouter/routerand check that the declaration file exists.This follows the PR objective that the package export must resolve both runtime and declaration artifacts.
🤖 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 `@scripts/smoke-dist.mjs` around lines 70 - 78, Update the smoke test around the router import to validate the public `@blockrun/clawrouter/router` export instead of importing dist/router/index.js directly. Preserve the existing route and default portfolio configuration checks, and additionally verify that dist/router/index.d.ts exists so both runtime and declaration artifacts are covered.package.json (1)
24-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPut the
typescondition beforeimport.TypeScript conditional exports are order-sensitive. With
importfirst, TypeScript can bypass the explicit declaration branch and rely on adjacent-file fallback. Puttypesfirst to make the public type contract unambiguous.Suggested export order
"./router": { - "import": "./dist/router/index.js", - "types": "./dist/router/index.d.ts" + "types": "./dist/router/index.d.ts", + "import": "./dist/router/index.js" }Verify the result with a NodeNext TypeScript consumer of
@blockrun/clawrouter/router.🤖 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 `@package.json` around lines 24 - 27, Update the "./router" conditional export in package.json so the "types" condition precedes "import", while preserving both existing targets. Verify the package with a NodeNext TypeScript consumer importing `@blockrun/clawrouter/router`.src/proxy.session-pinning.test.ts (1)
262-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert both shadow-routing methods.
The test can pass if both decisions use the same strategy. Assert that
shadows[0].executed.methodis"portfolio"andshadows[0].shadow.methodis"rules".🤖 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 `@src/proxy.session-pinning.test.ts` around lines 262 - 272, Extend the assertions in the session-pinning test to verify both shadow-routing decisions: assert that shadows[0].executed.method equals "portfolio" and shadows[0].shadow.method equals "rules", while preserving the existing shadow payload assertions.Source: Coding guidelines
src/router/portfolio.ts (3)
38-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the fallback weights from
DEFAULT_ROUTING_CONFIGinstead of duplicating them.
DEFAULT_PORTFOLIO_WEIGHTSrepeats the exact values insrc/router/config.tsLines 15-43. Two sources of truth exist for the same tuned numbers. A future tuning change inconfig.tsleaves this fallback stale without any test failure, because the fallback only applies whenoptions.config.portfoliois undefined.
config.tsimports only./types.js, so importing it here introduces no module cycle.♻️ Proposed deduplication
+import { DEFAULT_ROUTING_CONFIG } from "./config.js"; + -const DEFAULT_PORTFOLIO_WEIGHTS = { - auto: { quality: 0.47, capability: 0.2, cost: 0.18, speed: 0.07, reliability: 0.03, legacy: 0.05 }, - eco: { quality: 0.36, capability: 0.2, cost: 0.28, speed: 0.1, reliability: 0.04, legacy: 0.02 }, - premium: { quality: 0.58, capability: 0.2, cost: 0.08, speed: 0.06, reliability: 0.06, legacy: 0.02 }, - highStakesBoost: { quality: 0.08, reliability: 0.05 }, - latencySensitiveSpeedBoost: 0.08, - affinityFloorGap: { auto: 0.1, eco: 0.22, premium: 0.05 }, -} as const; +const DEFAULT_PORTFOLIO_WEIGHTS = DEFAULT_ROUTING_CONFIG.portfolio!;🤖 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 `@src/router/portfolio.ts` around lines 38 - 45, Replace the duplicated numeric values in DEFAULT_PORTFOLIO_WEIGHTS with values derived from DEFAULT_ROUTING_CONFIG imported from the router configuration module. Preserve the existing portfolio shape, including highStakesBoost, latencySensitiveSpeedBoost, and affinityFloorGap, while ensuring changes to DEFAULT_ROUTING_CONFIG automatically update this fallback.
589-593: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the precomputed
costsarray, and note thatcapabilityScorecannot discriminate.Two points in the scoring map:
Line 591 recomputes
estimatedCost(...)for each candidate. Line 582 already built thecostsarray over the samecandidatesin the same order. Usecosts[index].Line 593 sets
capabilityScorefromisEligible(model, features, maxOutputTokens). Wheneligible.length > 0at Line 562, every entry incandidatesalready passedisEligible.capabilityScoreis then1for every candidate, so thecapabilityweight (0.2 in all three profiles) adds the same constant to every score and never changes the ranking. The term only varies on the degenerate path where no model was eligible. Either document this or fold the weight into the discriminating dimensions.♻️ Proposed reuse of `costs`
const rankedEntries = candidates .map((model, index) => { - const cost = estimatedCost(model, options, features.estimatedInputTokens, maxOutputTokens); + const cost = costs[index]; const costScore = Number.isFinite(cost) && maxCost > minCost ? 1 - (cost - minCost) / (maxCost - minCost) : 0.5;🤖 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 `@src/router/portfolio.ts` around lines 589 - 593, Update the ranking map in rankedEntries to reuse costs[index] instead of recomputing estimatedCost for each candidate. Since candidates is derived from eligible models, capabilityScore is constant whenever candidates are non-empty; document this invariant or remove/fold the capability weight so scoring only includes discriminating dimensions, while preserving the no-eligible-model behavior.
632-640: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
tool_agentbranch is dead code; both arms are identical.The second and third arms of the ternary produce the same array:
[...scoredModels, ...eligibleCandidates.filter((model) => !scoredModels.includes(model))]The condition
features.taskType === "tool_agent" || (features.taskType === "tool_agent_parallel" && features.agentDomain !== "other")therefore has no effect onranked. Either the branch is leftover from a refactor, or an intended tool-agent-specific ordering was lost.♻️ Proposed simplification if no distinct ordering is intended
const ranked = features.agentDomain === "web_research" ? [ ...scoredModels, ...webResearchFallbackOrder.filter((model) => eligibleCandidates.includes(model) && !scoredModels.includes(model)), ...eligibleCandidates.filter((model) => !scoredModels.includes(model) && !webResearchFallbackOrder.includes(model)), ] - : features.taskType === "tool_agent" || (features.taskType === "tool_agent_parallel" && features.agentDomain !== "other") - ? [ - ...scoredModels, - ...eligibleCandidates.filter((model) => !scoredModels.includes(model)), - ] : [ ...scoredModels, ...eligibleCandidates.filter((model) => !scoredModels.includes(model)), ];If a distinct tool-agent ordering was intended, tell me and I can draft it.
🤖 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 `@src/router/portfolio.ts` around lines 632 - 640, Remove the redundant ternary condition around the ranked-model construction and assign the shared array expression directly to the result. Update the surrounding logic in the visible portfolio ranking flow so taskType and agentDomain no longer control identical branches, while preserving the existing scoredModels-first ordering and eligible-candidate deduplication.src/router/portfolio.test.ts (1)
32-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
ecoandpremiumrouting profiles.Every test in this file uses
routingProfile: "auto"or omits the field, which defaults toautoatsrc/router/portfolio.tsLine 563. The newportfolioconfig block introduces materially different behavior per profile:
affinityFloorGap:auto0.1,eco0.22,premium0.05.- Weight vectors differ, most notably
cost(0.18 / 0.28 / 0.08) andquality(0.47 / 0.36 / 0.58).None of that is exercised. A regression in the
ecoorpremiumweights would pass this suite. Add at least one test per profile that asserts a different selected model for the same prompt, so the profile weighting is locked in.As per coding guidelines: "Use Vitest tests to cover error and lifecycle resilience, end-to-end tool ID sanitization, and Docker installation, edge-case, and integration behavior where applicable."
🤖 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 `@src/router/portfolio.test.ts` around lines 32 - 48, Add Vitest coverage in the PortfolioStrategy tests for both routingProfile "eco" and "premium", using the same prompt and routing inputs while explicitly setting each profile. Assert that each profile selects its expected model and that the selected models differ from the auto-profile result, exercising the profile-specific weighting and affinity behavior.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.
Inline comments:
In `@src/proxy.ts`:
- Around line 5088-5100: Update the candidate pipeline around
filterCandidatesByCapacity and every later prependStickyExplicitModel call so
filtering can remove an ineligible stickyExplicitModel without reintroducing it.
Keep the sticky model first before capability filtering, but only retain it
afterward if it remains in the filtered candidates; apply the same behavior
after exclusion, tool, and vision filters.
- Around line 4417-4424: The onShadowRouted telemetry currently reports
routingDecision before session pin resolution, so comparison.executed may not
identify the model that served the request. Move the onShadowRouted callback
until after the session-selection logic that can replace routingDecision, while
preserving its existing payload fields and comparison behavior.
- Around line 4389-4390: Update RulesStrategy’s structured-output classification
to honor requiresStructuredOutput in addition to JSON-related systemPrompt
checks, ensuring requests with response_format reach structuredOutputMinTier and
rules-based shadow results use the same determination.
In `@src/router/model-profiles.ts`:
- Around line 29-31: Move the liveProfiles import from
./model-profiles.generated.json above the exported LIVE_MODEL_PROFILES
declaration in model-profiles.ts, preserving the existing export implementation
and project import ordering.
In `@src/router/portfolio.ts`:
- Around line 103-107: Bound classification regex inputs in classifyTask using
the existing classifier.promptTruncationChars setting: derive truncated
scannedPrompt and scannedFullText values, use them for all feature regexes and
match-based signals including clueConnectors, multipleChoiceSignals, and
numericSignals, while keeping estimatedInputTokens based on the complete
fullText.
- Around line 1-8: Format all affected router files with Prettier: run it on
src/router/portfolio.ts, src/router/model-profiles.ts,
src/router/tool-intent.ts, src/router/tool-intent.test.ts, and
src/router/portfolio.test.ts, or the entire src/router directory. In
src/router/model-profiles.ts, move the trailing import to the top before
formatting; ensure PortfolioStrategy.route has consistent indentation and its
sort chain is properly terminated. The formatter should also wrap long regex
literals and remove the stray space in tool-intent.test.ts.
- Around line 530-539: Validate the parsed timestamp in the profile-scoring flow
before calculating ageDays: if Date.parse(profile.measuredAt) is not finite,
return undefined for that profile. Keep valid timestamps and all subsequent
freshness and ranking calculations unchanged.
- Around line 557-573: Guard the candidate-selection flow after constructing
`chain`: when no candidate exists, return the rules decision using `base.model`
before calculating affinity or indexing `eligibleCandidates`. Replace the unsafe
`base.tierConfigs!` access with appropriate handling for missing tier
configuration, while preserving the existing eligibility and affinity behavior
when candidates are available.
In `@src/router/tool-intent.test.ts`:
- Around line 19-28: Update inferToolRequirement to immediately return false
when toolChoice is "none", before prose matching, preserving required,
function-selection, and auto behavior. Extend the tool-choice tests with a
tool_choice "none" case using a tool-relevant prompt, and add coverage that
passes a non-empty systemPrompt to validate the existing routing behavior.
In `@src/router/tool-intent.ts`:
- Around line 26-31: Update inferToolRequirement so explicitTool and webAction
are evaluated against prompt only, preventing tool descriptions in systemPrompt
from triggering action detection; retain combined text for codeEnvironment and
statefulAction as appropriate. Add a regression test covering a tool-describing
system prompt with a factual user question that must not be classified as
requiring a tool.
---
Nitpick comments:
In `@package.json`:
- Around line 24-27: Update the "./router" conditional export in package.json so
the "types" condition precedes "import", while preserving both existing targets.
Verify the package with a NodeNext TypeScript consumer importing
`@blockrun/clawrouter/router`.
In `@scripts/smoke-dist.mjs`:
- Around line 70-78: Update the smoke test around the router import to validate
the public `@blockrun/clawrouter/router` export instead of importing
dist/router/index.js directly. Preserve the existing route and default portfolio
configuration checks, and additionally verify that dist/router/index.d.ts exists
so both runtime and declaration artifacts are covered.
In `@src/proxy.session-pinning.test.ts`:
- Around line 262-272: Extend the assertions in the session-pinning test to
verify both shadow-routing decisions: assert that shadows[0].executed.method
equals "portfolio" and shadows[0].shadow.method equals "rules", while preserving
the existing shadow payload assertions.
In `@src/router/portfolio.test.ts`:
- Around line 32-48: Add Vitest coverage in the PortfolioStrategy tests for both
routingProfile "eco" and "premium", using the same prompt and routing inputs
while explicitly setting each profile. Assert that each profile selects its
expected model and that the selected models differ from the auto-profile result,
exercising the profile-specific weighting and affinity behavior.
In `@src/router/portfolio.ts`:
- Around line 38-45: Replace the duplicated numeric values in
DEFAULT_PORTFOLIO_WEIGHTS with values derived from DEFAULT_ROUTING_CONFIG
imported from the router configuration module. Preserve the existing portfolio
shape, including highStakesBoost, latencySensitiveSpeedBoost, and
affinityFloorGap, while ensuring changes to DEFAULT_ROUTING_CONFIG automatically
update this fallback.
- Around line 589-593: Update the ranking map in rankedEntries to reuse
costs[index] instead of recomputing estimatedCost for each candidate. Since
candidates is derived from eligible models, capabilityScore is constant whenever
candidates are non-empty; document this invariant or remove/fold the capability
weight so scoring only includes discriminating dimensions, while preserving the
no-eligible-model behavior.
- Around line 632-640: Remove the redundant ternary condition around the
ranked-model construction and assign the shared array expression directly to the
result. Update the surrounding logic in the visible portfolio ranking flow so
taskType and agentDomain no longer control identical branches, while preserving
the existing scoredModels-first ordering and eligible-candidate deduplication.
🪄 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: f31eaf15-f3a4-48c5-90b7-900f673f80ce
⛔ Files ignored due to path filters (1)
src/router/model-profiles.generated.jsonis excluded by!**/*.generated.*
📒 Files selected for processing (18)
package.jsonscripts/smoke-dist.mjssrc/index.tssrc/proxy.session-pinning.test.tssrc/proxy.tssrc/router/config.tssrc/router/index.tssrc/router/model-profiles.tssrc/router/portfolio.test.tssrc/router/portfolio.tssrc/router/selector.test.tssrc/router/selector.tssrc/router/strategy.test.tssrc/router/strategy.tssrc/router/tool-intent.test.tssrc/router/tool-intent.tssrc/router/types.tstsup.config.ts
| requiresStructuredOutput: | ||
| typeof parsed.response_format === "object" && parsed.response_format !== null, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply requiresStructuredOutput in RulesStrategy.
The proxy passes this protocol signal, but RulesStrategy only checks JSON-related text in systemPrompt. A request with response_format and no matching system prompt can stay below structuredOutputMinTier when strategy: "rules" is active. This also makes rules-based shadow results inconsistent with the requested output constraint.
Proposed fix
- const hasStructuredOutput = systemPrompt ? /json|structured|schema/i.test(systemPrompt) : false;
+ const hasStructuredOutput =
+ options.requiresStructuredOutput ||
+ (systemPrompt ? /json|structured|schema/i.test(systemPrompt) : false);🤖 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 `@src/proxy.ts` around lines 4389 - 4390, Update RulesStrategy’s
structured-output classification to honor requiresStructuredOutput in addition
to JSON-related systemPrompt checks, ensuring requests with response_format
reach structuredOutputMinTier and rules-based shadow results use the same
determination.
| it("honors the OpenAI tool_choice contract", () => { | ||
| expect(inferToolRequirement("Retrieve the account details.", undefined, "required")).toBe(true); | ||
| expect( | ||
| inferToolRequirement("Retrieve the account details.", undefined, { | ||
| type: "function", | ||
| function: { name: "get_account" }, | ||
| }), | ||
| ).toBe(true); | ||
| expect(inferToolRequirement("What is 17 times 9?", undefined, "auto")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add coverage for tool_choice: "none" and for a non-empty systemPrompt.
The tool_choice block asserts "required", a function selection, and "auto". It does not assert "none". The OpenAI contract states that "none" forbids tool calls, but inferToolRequirement ignores that value and falls through to prose matching. A prompt such as "Cancel my flight booking and refund the ticket." with tool_choice: "none" currently returns true.
The systemPrompt parameter is never exercised with a value in this file. That parameter carries the over-routing risk noted on src/router/tool-intent.ts Lines 26-31.
💚 Proposed added assertions
expect(inferToolRequirement("What is 17 times 9?", undefined, "auto")).toBe(false);
+ expect(
+ inferToolRequirement("Cancel my flight booking and refund the ticket.", undefined, "none"),
+ ).toBe(false);
+ });
+
+ it("does not treat a tool-describing system prompt as a tool requirement", () => {
+ const systemPrompt = "You can use the web_search tool to look up documentation.";
+ expect(inferToolRequirement("What is 17 times 9?", systemPrompt)).toBe(false);
});The "none" assertion fails against the current implementation. Add the short-circuit in src/router/tool-intent.ts alongside it.
🤖 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 `@src/router/tool-intent.test.ts` around lines 19 - 28, Update
inferToolRequirement to immediately return false when toolChoice is "none",
before prose matching, preserving required, function-selection, and auto
behavior. Extend the tool-choice tests with a tool_choice "none" case using a
tool-relevant prompt, and add coverage that passes a non-empty systemPrompt to
validate the existing routing behavior.
Source: Coding guidelines
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/router/portfolio.ts (1)
1113-1133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo branches of the
rankedternary are identical. Thetool_agent/tool_agent_parallelbranch at Lines 1126-1129 and the default branch at Lines 1130-1133 build the same array. The condition at Lines 1124-1125 has no effect. Collapse it to a single non-web-research branch, or implement the intended distinct ordering.♻️ Proposed simplification
const ranked = features.agentDomain === "web_research" ? [ ...scoredModels, ...webResearchFallbackOrder.filter( (model) => eligibleCandidates.includes(model) && !scoredModels.includes(model), ), ...eligibleCandidates.filter( (model) => !scoredModels.includes(model) && !webResearchFallbackOrder.includes(model), ), ] - : features.taskType === "tool_agent" || - (features.taskType === "tool_agent_parallel" && features.agentDomain !== "other") - ? [ - ...scoredModels, - ...eligibleCandidates.filter((model) => !scoredModels.includes(model)), - ] - : [ - ...scoredModels, - ...eligibleCandidates.filter((model) => !scoredModels.includes(model)), - ]; + : [...scoredModels, ...eligibleCandidates.filter((model) => !scoredModels.includes(model))];🤖 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 `@src/router/portfolio.ts` around lines 1113 - 1133, Collapse the redundant tool-agent ternary branch in the ranked construction: after the web_research case, use one shared non-web-research array combining scoredModels with remaining eligibleCandidates. Remove the ineffective features.taskType condition while preserving the existing ordering.
🤖 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 `@src/router/portfolio.ts`:
- Around line 1113-1133: Collapse the redundant tool-agent ternary branch in the
ranked construction: after the web_research case, use one shared
non-web-research array combining scoredModels with remaining eligibleCandidates.
Remove the ineffective features.taskType condition while preserving the existing
ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1657c6f9-206b-4847-a210-9e9dbccc219d
📒 Files selected for processing (9)
scripts/smoke-dist.mjssrc/proxy.tssrc/router/model-profiles.tssrc/router/portfolio.test.tssrc/router/portfolio.tssrc/router/selector.test.tssrc/router/selector.tssrc/router/tool-intent.test.tssrc/router/tool-intent.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/smoke-dist.mjs
- src/router/tool-intent.ts
- src/router/selector.test.ts
- src/router/selector.ts
- src/router/tool-intent.test.ts
- src/router/model-profiles.ts
- src/router/portfolio.test.ts
- src/proxy.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/router/tool-intent.ts (1)
30-43: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winBound the text scanned by the tool-intent regexes.
inferToolRequirementscans the complete user prompt four times.proxyRequestcalls it before request compression. An unbounded prompt cannot meet the 1 ms routing budget. Sample a fixed prefix and suffix before matching, as the portfolio classifier does.As per coding guidelines,
src/**/*.{ts,tsx}must keep model-routing dimension scoring local and under 1 ms.🤖 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 `@src/router/tool-intent.ts` around lines 30 - 43, Update inferToolRequirement to match the explicitTool, codeEnvironment, webAction, and statefulAction regexes against a bounded sample of prompt text rather than the complete prompt. Build the sample from a fixed-size prefix and suffix, reusing the portfolio classifier’s established bounds, then preserve the existing four-match boolean result and ensure proxyRequest benefits before compression.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.
Inline comments:
In `@src/proxy.ts`:
- Around line 5100-5109: Update the capacity-filtering flow around
filterCandidatesByCapacity so an entirely capacity-ineligible fullChain produces
an empty candidate result rather than restoring fullChain. Ensure the caller
emits a clear capacity error and does not retry rejected models, while
preserving normal filtering for eligible candidates; add Vitest coverage for the
case where every configured candidate is too small.
- Around line 4580-4589: Wrap the optional onShadowRouted callback invocation in
the routing flow with isolated error handling so callback exceptions cannot
escape the outer try or prevent modified request-body serialization and upstream
completion. Preserve the existing callback payload and continue routing after
failures. Add a Vitest test in the applicable *.test.ts or *.test.tsx file where
onShadowRouted throws and verify the sampled upstream request still succeeds.
---
Outside diff comments:
In `@src/router/tool-intent.ts`:
- Around line 30-43: Update inferToolRequirement to match the explicitTool,
codeEnvironment, webAction, and statefulAction regexes against a bounded sample
of prompt text rather than the complete prompt. Build the sample from a
fixed-size prefix and suffix, reusing the portfolio classifier’s established
bounds, then preserve the existing four-match boolean result and ensure
proxyRequest benefits before compression.
🪄 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: 1cc4af9f-a42e-4867-8056-5f2a2d8a0d03
📒 Files selected for processing (9)
src/proxy.session-pinning.test.tssrc/proxy.tssrc/router/model-profiles.tssrc/router/portfolio.test.tssrc/router/portfolio.tssrc/router/strategy.test.tssrc/router/strategy.tssrc/router/tool-intent.test.tssrc/router/tool-intent.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/router/tool-intent.test.ts
- src/router/model-profiles.ts
- src/router/strategy.test.ts
- src/router/portfolio.ts
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 `@src/proxy.session-pinning.test.ts`:
- Around line 353-360: Update the test around the onShadowRouted callback to
increment an invocation counter before throwing, then assert the counter equals
one after postChat. Preserve the existing response-status and receivedModels
assertions while explicitly verifying the throwing callback was invoked.
🪄 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: c92c7036-8236-4f66-9c98-1284db5e1ec4
📒 Files selected for processing (4)
src/proxy.session-pinning.test.tssrc/proxy.tssrc/router/selector.test.tssrc/router/selector.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/router/selector.test.ts
- src/router/selector.ts
- src/proxy.ts
# Conflicts: # src/router/config.ts
…d liveness Review follow-up on BlockRunAI#238. Three blocking items, all mechanical; the router extraction and the portfolio policy are unchanged. 1. Committed dist/ was stale relative to the pinned router-core. dist/router/index.js carried "free/seed-oss-36b" in three fallback chains (lines 3013/3131/3221) with the pre-fix comments intact. That model went HTTP 410 EOL at NVIDIA on 2026-08-03 and v0.12.241 removed it. The pin itself was already correct: router-core 6a790eb ("fix: retire unavailable seed fallback") fixes all three sites in config.ts, and its own dist is clean. Only ClawRouter's committed artifact was built against an older snapshot. Rebuilt; the chains are now clean. This mattered because BlockRun server-redirects retired free ids, so routing to one SILENTLY DEFEATS /exclude: the caller excludes a model, the router hands it the request anyway, and the gateway answers from the redirect target. 2. @blockrun/router-core moved to devDependencies. tsup's noExternal inlines it (dist/router/index.js line 3 is "// node_modules/@blockrun/router-core/dist/index.js"), so nothing imports it at runtime. In dependencies it made every `npm install @blockrun/clawrouter` fetch a codeload.github.com tarball at install time for code already in the bundle. The integrity hash protects `npm ci` here, but the published package does not ship our lockfile: consumers resolve the bare URL from package.json with no verification. It also made installs fail whenever GitHub is unreachable or the org is renamed, which has happened before. The published tarball now declares zero non-registry dependencies again. 3. Version 0.12.241 -> 0.12.242. 0.12.241 is already on npm. publish.yml guards on `pkg != npm`, so merging at 0.12.241 would land on main and silently never publish. Also adds src/router/free-model-liveness.test.ts. Router selection coverage left this repo with the code (713 tests here vs 731 on main, selector.test.ts and strategy.test.ts deleted, none added), and item 1 is what walked through the gap. The guard walks every tier container's primary and fallback entries and asserts each free/* id is live in src/top-models.json, plus the two deliberate gpt-oss defaults. The allowed set is derived from the picker, not hard-coded, so a future free-tier resync updates it automatically. Verified to FAIL against the regression: re-injecting seed-oss into one chain produces "tiers.SIMPLE.fallback[3] -> free/seed-oss-36b" and fails two of the three assertions. A third assertion fails loudly if the config shape changes and the walk stops finding tiers, so it cannot pass vacuously. Verified: typecheck, lint, prettier, build + dist smoke all clean. 716 tests pass (713 + 3 new). Packed artifact confirmed to declare no URL dependency, and dist/ confirmed free of seed-oss routing entries; the four remaining mentions in dist/index.js are the deliberate pins and catalog entry that v0.12.241 kept routable.
|
Reviewed and pushed three fixes to this branch ( 1. The committed
|
…action, stale-build fix
2 participants
Summary
BlockRunAI/router-coreand pin this product to immutable commit6a790eb@blockrun/clawrouter/routeras a backwards-compatible re-export for existing SDK consumersstrategy: "rules"as an immediate rollback and optional local-only shadow comparison with no second model call0.12.241Why
The previous Auto path used a fixed tier primary after rule classification. The evaluated portfolio policy improves model selection for agent workflows while keeping routing local and deterministic. In the frozen 100-task, three-arm Franklin evaluation, the new policy scored 57% versus 49% for the previous router and reduced normalized cost per successful task by 6.44%.
Extracting the selector is also a cleaner ownership boundary: Router Core owns classification, hard constraints, portfolio scoring, and fallback ordering; ClawRouter owns its gateway, wallet, payment, proxy, session, and live model catalog. The benchmark remains a separate consumer and is not shipped in either product.
Safety and rollout
routing.strategy = "rules"tool_choice: "none"is authoritative, and host tool descriptions do not create false per-turn tool requirementsValidation
blockrun/autoProxy request completed end to end; the existing insufficient-balance path correctly fell back to the configured free modelmainmodel-lifecycle update is synchronized: the retiredseed-oss-36bfallback was removed once in Router Core and inherited by both productsSummary by CodeRabbit
New Features
Bug Fixes
Tests