feat: smart auto-routing (per-turn simple-vs-strong model selection) - #1734
Conversation
📝 WalkthroughWalkthroughAdds an opt-in smart-routing feature that normalizes configuration, classifies turns, pins per-turn routing decisions, retries some routed failures on the strong model, and exposes routing state through ChangesSmart Auto-Routing End-to-End
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/smartroute/index.test.ts (1)
1-109:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix type narrowing to resolve typecheck failures.
The pipeline shows 11 typecheck errors because the tests access
res.valuewithout first narrowing theLocalCommandResulttype. SinceLocalCommandResultis a discriminated union, you must checkres.type === 'text'before accessing.value:test('status with no config shows disabled and available keys', async () => { const ctx = makeContext() const res = await call('', ctx) + expect(res.type).toBe('text') + if (res.type !== 'text') throw new Error('Expected text result') expect(res.value).toContain('status: disabled') expect(res.value).toContain('mini, main') })Apply this pattern to all test cases that access
res.value.🤖 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/commands/smartroute/index.test.ts` around lines 1 - 109, The tests access res.value without type narrowing the LocalCommandResult discriminated union, causing typecheck failures. Before accessing res.value in each test case where the call function is invoked (in tests like 'status with no config shows disabled and available keys', 'on without both roles set is rejected', 'simple/strong with no value argument is rejected', 'setting a role to an unknown key is rejected with available keys', 'enabling with simple cheaper than strong gives no warning', 'warns when the simple model is not cheaper than the strong model', and 'off disables'), add a type guard check to verify res.type === 'text' before accessing the .value property.
🤖 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/services/api/smartRouting/index.test.ts`:
- Around line 114-139: The three test cases 'short non-first turn routes
simple', 'first turn routes strong (routeModel turnNumber===1 guard)', and
'strong-signal prompt routes strong' are failing because decideTurnModel calls
isModelAllowed(model) to enforce org allowlist restrictions, but these tests do
not mock that function. Since the mock is not present, the real isModelAllowed
implementation runs and rejects the test models (gpt-5-mini and gpt-5), causing
routing to be disabled and returning routed: false instead of the expected
routed: true. Add a mock for isModelAllowed before each of these three test
cases to allow the test model names to pass the allowlist check. You can either
add the mock individually before each test or create a beforeEach block that
mocks isModelAllowed for the scope of these tests only.
In `@src/utils/conversationRecovery.test.ts`:
- Around line 409-444: The test cases stripThinkingBlocksIfProviderAllows
preserves thinking for preserve-reasoning 3P and
stripThinkingBlocksIfProviderAllows strips thinking for generic OpenAI 3P both
use as any type assertions to bypass TypeScript type checking when constructing
test message objects. Replace these as any casts with properly-typed test
fixtures that match the NormalizedMessage shape expected by
stripThinkingBlocksIfProviderAllows. Create a typed helper function or factory
that constructs the assistant message objects with correct types, ensuring the
function parameter and the content array elements conform to the actual expected
types without requiring type assertions.
In `@src/utils/conversationRecovery.ts`:
- Around line 228-256: The provider-detection logic for stripping thinking
blocks is duplicated between the new stripThinkingBlocksIfProviderAllows
function and the deserializeMessagesWithInterruptDetection function. Replace the
duplicated provider-gate logic in deserializeMessagesWithInterruptDetection (the
section that mirrors the implementation in stripThinkingBlocksIfProviderAllows)
with a direct call to stripThinkingBlocksIfProviderAllows to eliminate code
duplication and maintain a single source of truth for this logic.
---
Outside diff comments:
In `@src/commands/smartroute/index.test.ts`:
- Around line 1-109: The tests access res.value without type narrowing the
LocalCommandResult discriminated union, causing typecheck failures. Before
accessing res.value in each test case where the call function is invoked (in
tests like 'status with no config shows disabled and available keys', 'on
without both roles set is rejected', 'simple/strong with no value argument is
rejected', 'setting a role to an unknown key is rejected with available keys',
'enabling with simple cheaper than strong gives no warning', 'warns when the
simple model is not cheaper than the strong model', and 'off disables'), add a
type guard check to verify res.type === 'text' before accessing the .value
property.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 39570f9c-171f-4330-b1a9-5196b79794e4
📒 Files selected for processing (21)
README.mddocs/smart-routing.mdsrc/commands.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.test.tssrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/query.tssrc/services/api/agentRouting.tssrc/services/api/openaiShim.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/index.tssrc/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/settings.tssrc/utils/conversationRecovery.test.tssrc/utils/conversationRecovery.tssrc/utils/settings/types.tsweb/src/data/commands.tsweb/src/data/configuration.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (18)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tssrc/commands.tssrc/commands/smartroute/index.test.tssrc/services/api/openaiShim.tssrc/utils/settings/types.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.tssrc/query.ts
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
chalkfor terminal color in CLI code
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tssrc/commands/smartroute/index.test.tssrc/services/api/openaiShim.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/commands/smartroute/index.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.ts
{src/services/**/*.ts,src/utils/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
execafor child processes
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tssrc/services/api/openaiShim.tssrc/utils/settings/types.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.ts
{src/integrations/**/*.ts,src/services/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Test the exact provider/model path you changed when possible for provider modifications
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tssrc/services/api/openaiShim.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.ts
**/*.{ts,tsx,js,jsx,py,json,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tsweb/src/data/commands.tssrc/commands.tssrc/commands/smartroute/index.test.tsREADME.mdsrc/services/api/openaiShim.tsdocs/smart-routing.mdsrc/utils/settings/types.tsweb/src/data/configuration.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.tssrc/query.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/index.test.tssrc/utils/conversationRecovery.test.ts
**/*.test.{ts,tsx,js}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test the exact provider/model path you changed when possible
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/index.test.tssrc/utils/conversationRecovery.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tsweb/src/data/commands.tssrc/commands.tssrc/commands/smartroute/index.test.tssrc/services/api/openaiShim.tssrc/utils/settings/types.tsweb/src/data/configuration.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.tssrc/query.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow TypeScript strict mode and type safety practices by running typecheck before submitting
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tsweb/src/data/commands.tssrc/commands.tssrc/commands/smartroute/index.test.tssrc/services/api/openaiShim.tssrc/utils/settings/types.tsweb/src/data/configuration.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.tssrc/query.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tsweb/src/data/commands.tssrc/commands.tssrc/commands/smartroute/index.test.tsREADME.mdsrc/services/api/openaiShim.tsdocs/smart-routing.mdsrc/utils/settings/types.tsweb/src/data/configuration.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.tssrc/query.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tssrc/services/api/openaiShim.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/index.test.tssrc/utils/conversationRecovery.test.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
- Python exists for legacy/local-provider helper code. Do not add new Python code or expand Python-based features unless a maintainer explicitly approves that direction.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.- `src/integration...
Files:
src/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/settings.tsweb/src/data/commands.tssrc/commands.tssrc/commands/smartroute/index.test.tsREADME.mdsrc/services/api/openaiShim.tsdocs/smart-routing.mdsrc/utils/settings/types.tsweb/src/data/configuration.tssrc/services/api/smartRouting/settings.test.tssrc/utils/conversationRecovery.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/utils/conversationRecovery.test.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/services/api/agentRouting.tssrc/services/api/smartRouting/index.tssrc/query.ts
web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Web changes must pass web typecheck and build via 'bun run web:typecheck' and 'bun run web:build'
Files:
web/src/data/commands.tsweb/src/data/configuration.ts
web/**
⚙️ CodeRabbit configuration file
web/**: Review browser extension changes for content-script isolation, message validation, cross-origin assumptions, permission surfaces, and failures that could leak prompts or credentials.
Files:
web/src/data/commands.tsweb/src/data/configuration.ts
{src/commands/**/*.ts,src/entrypoints/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use
commanderfor CLI argument parsing
Files:
src/commands/smartroute/index.test.tssrc/commands/smartroute/index.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
README.mddocs/smart-routing.md
docs/**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update docs when setup, commands, or user-facing behavior changes
Files:
docs/smart-routing.md
🪛 GitHub Actions: PR Checks / 1_smoke-and-tests.txt
src/services/api/smartRouting/index.test.ts
[error] 115-120: Test failed (Jest): expect(received).toMatchObject(expected) in decideTurnModel > disabled settings → routed:false. Expected { routed: true, complexity: 'simple', model: 'gpt-5-mini', strongModel: 'gpt-5' } but received { routed: false } at index.test.ts:120:15.
🪛 GitHub Actions: PR Checks / 2_typecheck.txt
src/commands/smartroute/index.test.ts
[error] 41-41: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 42-42: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 48-48: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 70-70: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 77-77: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 78-78: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 85-85: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 86-86: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 96-96: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 97-97: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
[error] 103-103: tsc (TypeScript) error TS2339: Property 'value' does not exist on type 'LocalCommandResult'. Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }'.
🪛 GitHub Actions: PR Checks / smoke-and-tests
src/services/api/smartRouting/index.test.ts
[error] 115-120: Test failed in decideTurnModel. expect(received).toMatchObject(expected) mismatched: expected { routed: true, complexity: 'simple', model: 'gpt-5-mini', strongModel: 'gpt-5' } but received { routed: false }.
🪛 GitHub Actions: PR Checks / typecheck
src/commands/smartroute/index.test.ts
[error] 41-41: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'. (Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }').
[error] 42-42: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'. (Property 'value' does not exist on type '{ type: "compact"; compactionResult: CompactionResult; displayText?: string | undefined; nextInput?: string | undefined; submitNextInput?: boolean | undefined; }').
[error] 48-48: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 70-70: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 77-77: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 78-78: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 85-85: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 86-86: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 96-96: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 97-97: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
[error] 103-103: tsc --noEmit: TS2339 Property 'value' does not exist on type 'LocalCommandResult'.
🪛 GitHub Check: smoke-and-tests
src/services/api/smartRouting/index.test.ts
[failure] 138-138: error: expect(received).toMatchObject(expected)
{
- "complexity": "strong",
- "model": "gpt-5",
- "routed": true,
- "routed": false,
}
- Expected - 3
-
Received + 1
at <anonymous> (/home/runner/work/openclaude/openclaude/src/services/api/smartRouting/index.test.ts:138:15)
[failure] 129-129: error: expect(received).toMatchObject(expected)
{
- "complexity": "strong",
- "model": "gpt-5",
- "routed": true,
- "routed": false,
}
- Expected - 3
-
Received + 1
at <anonymous> (/home/runner/work/openclaude/openclaude/src/services/api/smartRouting/index.test.ts:129:15)
[failure] 120-120: error: expect(received).toMatchObject(expected)
{
- "complexity": "simple",
- "model": "gpt-5-mini",
- "routed": true,
- "strongModel": "gpt-5",
- "routed": false,
}
- Expected - 4
-
Received + 1
at <anonymous> (/home/runner/work/openclaude/openclaude/src/services/api/smartRouting/index.test.ts:120:15)
🔇 Additional comments (23)
README.md (1)
191-191: LGTM — Smart Auto-Routing doc link placement is correct.The addition fits naturally under "Advanced and source-build guides" as an optional experimental feature. The link format is valid for relative navigation.
docs/smart-routing.md (1)
1-58: Documentation is accurate and complete.Verified all major claims against the implemented
/smartroutecommand handler,decideTurnModellogic, and query-loop integration:
- Setup example and schema match configured defaults ✓
- Command table (status/on/off/simple/strong) matches handler branches ✓
- Env vars match expected naming (
OPENCLAUDE_SMART_ROUTING*) ✓- Behavior: one-per-turn decision, fallback to strong on retryable errors, allowlist coercion with session disable, re-enable via
/smartroute on— all documented correctly ✓- /cost summary telemetry mentioned as per PR scope ✓
- Experimental label and off-by-default flags are clear ✓
Minor non-blocking note: The docs don't detail the classifier heuristic (prompt length, code blocks, reasoning keywords), but this is intentional—the conservative failure mode ("routes to strong when unsure") is the key guarantee, and implementation details aren't a user concern. Appropriate level of abstraction.
src/utils/settings/types.ts (1)
783-808: LGTM!src/services/api/smartRouting/settings.ts (1)
1-84: LGTM!src/services/api/smartRouting/settings.test.ts (1)
1-112: LGTM!src/services/api/agentRouting.ts (1)
163-172: LGTM!src/services/api/smartRouting/resolveConfig.ts (1)
1-84: LGTM!src/services/api/smartRouting/resolveConfig.test.ts (1)
1-94: LGTM!src/services/api/smartRouting/index.ts (1)
1-277: LGTM!src/services/api/smartRouting/index.test.ts (1)
65-316: ⚡ Quick winTest coverage is comprehensive once the allowlist mock issue is fixed.
The remaining tests (deriveUserTurnNumber, extractLatestUserText, allowlist enforcement, retryability, pin-drop logic, tally tracking, and summary formatting) are well-structured and cover the expected edge cases. The session-scoped disable tracking tests (lines 158-213) correctly verify isolation and clearing behavior. The retryability tests (lines 217-234) match the intentional 404/429-retryable design documented in
index.tsline 132-137.src/cost-tracker.ts (1)
12-13: LGTM!Also applies to: 88-90, 294-299
src/commands/login/login.tsx (1)
4-6: LGTM!src/commands/smartroute/index.ts (1)
1-120: LGTM!src/commands.ts (1)
165-165: LGTM!Also applies to: 276-276
web/src/data/commands.ts (1)
95-95: LGTM!web/src/data/configuration.ts (1)
53-53: LGTM!Also applies to: 77-79
src/services/api/openaiShim.ts (1)
22-26: LGTM!src/query.ts (6)
106-106: LGTM!
126-138: LGTM!
370-379: LGTM!
774-828: LGTM!
946-952: LGTM!
1274-1331: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Stop presenting smart routing as provider-aware cost savings
src/services/api/smartRouting/index.ts:15
The feature is documented and surfaced as routing simple turns to the cheaper model for the current provider, and/smartrouteplus/costclaim to warn/estimate when the simple model is not actually cheaper. The implementation cannot know that today:getKnownInputCost()only canonicalizes the model name and looks it up in the static first-partyMODEL_COSTStable, without passing the active provider/profile/base URL/gateway route/account pricing or comparing observed billed usage. For OpenAI-compatible gateways and hosted providers, the same model id or alias can have different pricing from OpenClaude's first-party table, and unknown/private models just lose the estimate entirely. That means the UI can tell a multi-provider user that routing is saving money, or fail to warn that it is not, even though the current provider's pricing says otherwise. Please either make the cheaper-model check provider/route-aware, or narrow the docs/UI to say it only routes to the user-configured "simple" role and only estimates savings for pricing sources the code can actually verify.
|
@jatmn good catch, fixed in c114ff2. You're right that I didn't make the check provider/route-aware (your first option). OpenClaude has no per-provider or per-gateway pricing data today, and comparing observed billed usage is a real feature rather than a fix for this PR, so honest framing is the right scope here. Happy to file a follow-up for provider-aware pricing if you'd like it tracked separately. |
Classify once per user turn (transition===undefined), pin the decision in a loop-local, and apply the model-only route before the blocking-limit math. Enforce the org allowlist by calling isModelAllowed directly (coerce disallowed to strong; disable for the session if strong is also disallowed). Strip thinking history on a model change only under the provider gate (preserve-reasoning providers are left untouched). Export stripThinkingBlocksIfProviderAllows.
A simple-routed turn whose model call hits a retryable error retries once on the strong model, reusing the existing attemptWithFallback retry loop. Aborts and 4xx client errors propagate. Adds a session routing tally (simple/strong counts and simple->strong escalations) for the observability surface.
/smartroute shows status and sets/toggles the simple and strong roles from agentModels keys, warning when the simple model is not first-party-cheaper than the strong one. OPENCLAUDE_SMART_ROUTING(_SIMPLE/_STRONG) provide startup defaults; an explicit settings block overrides env.
Appends a session routing summary (turns simple/strong, simple->strong escalations) to /cost, with an estimated-savings line gated on first-party pricing and annotated unavailable for unknown third-party pricing. Per-turn cost is already attributed to the routed model via the existing per-model breakdown.
Without this, a turn's later continuation passes re-applied the pinned simple model after a fallback, re-triggering the same failure each pass. Re-pinning to strong keeps the rest of the turn on the recovered model.
- Add the KTD6 provider-swap guard: drop the per-turn routing pin when a mid-turn provider-fallback swap changes the active provider, so the old provider's model id is not replayed at the new endpoint (adversarial P1). - Reset the routing tally in resetCostState() so /cost does not show stale cross-session counts. - Don't emit the disabled-for-session notice on every turn when no sessionId is available (suppress instead of storm). - Document OPENCLAUDE_SMART_ROUTING* in the openaiShim env-var header. - Add tests: provider-swap-safe pin, undefined-session silence, /smartroute strong arm and no-value guard.
Register /smartroute in the web command catalog, add the smartRouting setting and OPENCLAUDE_SMART_ROUTING* env vars to the configuration reference, add a docs/smart-routing.md usage guide, and link it from the README.
…disabled set - /login used the raw bootstrap resetCostState, leaking the routing tally across an account switch; switch it to the cost-tracker wrapper. - Extract the provider-swap drop check as a pure, tested shouldDropPinForProviderSwap() and use it in the query loop. - Cap the disabledSessions set so a long-lived host can't grow it unbounded. - Document the 404/429 retry-by-design rationale; add tests for it. - Clarify the routedFallbackUsed per-turn scope and the apply-after-guard comment; document cross-provider role rejection and the re-enable path.
… mocks The decideTurnModel allowlist tests spied the global settings singleton, which let another file's leaked mock.module of modelAllowlist (agent.test.ts) flip isModelAllowed out from under them in the full suite. Spy isModelAllowed directly and restore it in afterEach so the tests are deterministic regardless of suite ordering.
- index.test.ts: pin the allowlist in the three happy-path decideTurnModel tests so they no longer inherit a leaked cross-file isModelAllowed mock (the CI test failure) - smartroute/index.test.ts: narrow the LocalCommandResult union via an expectText helper instead of reading .value off the union (the CI typecheck failure) - conversationRecovery.ts: route deserialize's thinking-strip gate through stripThinkingBlocksIfProviderAllows, removing the duplicated provider detection - conversationRecovery.test.ts: replace the two as-any fixtures with a shared typed factory
Smart routing's savings estimate and "simple isn't cheaper" warning were derived from the static first-party MODEL_COSTS table via getKnownInputCost, with no knowledge of the active provider, gateway, or account pricing. For a multi-provider user whose model ids happen to exist in that table but bill differently, the /cost summary and /smartroute warning stated a savings figure as if it reflected what they are actually charged. Narrow the copy instead of inventing provider-aware pricing the code cannot verify: the /cost line, the /smartroute warning, and docs/smart-routing.md now label the numbers as first-party reference pricing and note the active provider may bill differently. Tests assert the qualifier on every reworded branch so it cannot silently regress. No routing logic changed.
c114ff2 to
3b8fb27
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/commands/smartroute/index.ts`:
- Around line 27-30: The model-key lookup logic is duplicated in roleModelString
and getRoutingSummaryForDisplay, so update the smartroute command to reuse a
single shared resolver instead of repeating settings?.agentModels?.[key]?.model
?? key. Move or extract the lookup into a common helper used by both
roleModelString and the smartRouting display logic so the behavior stays
consistent and cannot drift.
- Around line 52-58: The persist flow is ignoring write failures from
updateSettingsForSource, so the in-memory state changes even when the on-disk
save fails. Update persist in smartroute/index.ts to capture the returned {
error } and make callers handle that result before composing the success
response. In the Smart routing command handlers that call persist, append a
warning to the user-facing message when persistence fails, using the existing
SmartRoutingSettings and updateSettingsForSource symbols to locate the affected
code.
- Around line 61-74: The `/smartroute` status output is reading
`current.simpleModel` and `current.strongModel` even when
`readSmartRouting(settings)` has enabled routing from environment-backed values.
Update the status block in the `smartroute` command to use the normalized
smart-routing values returned by `readSmartRouting` for the `simple` and
`strong` lines, so env-only setups no longer show `enabled` with both roles as
`(unset)`. Keep the existing `isSmartRoutingDisabledForSession` messaging and
`HELP` output unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 99ff2fa7-70eb-4e04-9d7d-c9f760666467
📒 Files selected for processing (16)
README.mddocs/smart-routing.mdsrc/commands.tssrc/commands/login/login.tsxsrc/commands/smartroute/index.test.tssrc/commands/smartroute/index.tssrc/cost-tracker.tssrc/query.tssrc/services/api/agentRouting.tssrc/services/api/openaiShim.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/index.tssrc/services/api/smartRouting/resolveConfig.test.tssrc/services/api/smartRouting/resolveConfig.tssrc/services/api/smartRouting/settings.test.tssrc/services/api/smartRouting/settings.ts
💤 Files with no reviewable changes (9)
- src/services/api/smartRouting/resolveConfig.test.ts
- src/services/api/smartRouting/settings.test.ts
- src/services/api/openaiShim.ts
- src/services/api/agentRouting.ts
- src/services/api/smartRouting/settings.ts
- src/services/api/smartRouting/resolveConfig.ts
- src/services/api/smartRouting/index.test.ts
- src/query.ts
- src/services/api/smartRouting/index.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: PR Checks / smoke-and-tests: feat: smart auto-routing (per-turn simple-vs-strong model selection)
Conclusion: failure
ool uses reason-aware result for hard_max
(pass) StreamingToolExecutor lifecycle tracking > already-aborted streaming tool uses reason-aware result for background [1.00ms]
##[endgroup]
##[group]src/services/tools/toolOrchestration.test.ts:
(pass) Bash commands with shell parser limitations are serialized [3.00ms]
##[endgroup]
##[group]src/services/tools/toolExecution.test.ts:
(pass) getSchemaValidationErrorOverride > returns actionable missing-skill error for SkillTool
(pass) getSchemaValidationErrorOverride > does not override unrelated tool schema failures
(pass) getSchemaValidationErrorOverride > does not override SkillTool when skill is present
(pass) getSchemaValidationErrorOverride > uses the actionable override for structured toolUseResult too
(pass) getReplayModifiedFiles > captures file-editing tool paths
(pass) getReplayModifiedFiles > captures Bash simulated sed edit paths
(pass) replay tool lifecycle records > records permission denied completions [1.00ms]
(pass) replay tool lifecycle records > records success completions with modified files
(pass) replay tool lifecycle records > records error completions
(pass) replay tool lifecycle records > classifies abort-shaped tool failures as cancelled
(pass) replay tool lifecycle records > captures the final executable input
(pass) replay tool lifecycle records > normalizes denied file-tool replay inputs to match allowed retry inputs
(pass) replay tool lifecycle records > records one error terminal status when post-call result processing fails [2.00ms]
(pass) query lifecycle tool-use cleanup > successful tool execution leaves no active lifecycle tool use [1.00ms]
(pass) query lifecycle tool-use cleanup > custom validation failure leaves no active lifecycle tool use
(pass) query lifecycle tool-use cleanup > schema validation failure does not end a lifecycle entry that never started [1.00ms]
(pass) query lifecycle tool-use cleanup > unknown tool does not end a lifecycle entry that never s...
GitHub Actions: PR Checks / 1_smoke-and-tests.txt: feat: smart auto-routing (per-turn simple-vs-strong model selection)
Conclusion: failure
ncomplete streamed Bash commands when finish_reason is length [1.00ms]
(pass) repairs truncated JSON objects even without command field [2.00ms]
(pass) preserves raw input for unknown plain string tool arguments [1.00ms]
(pass) preserves parsed string input for unknown JSON string tool arguments [1.00ms]
(pass) sanitizes malformed MCP tool schemas before sending them to OpenAI [2.00ms]
(pass) optional tool properties are not added to required[] — fixes Groq/Azure 400 tool_use_failed [1.00ms]
(pass) coalesces consecutive user messages to avoid alternation errors (issue `#202`) [1.00ms]
(pass) coalesces consecutive assistant messages preserving tool_calls (issue `#202`) [2.00ms]
(pass) non-streaming: reasoning_content emitted as thinking block only when content is null [1.00ms]
(pass) non-streaming: empty string content does not fall through to reasoning_content as text [1.00ms]
(pass) non-streaming: real content takes precedence over reasoning_content [1.00ms]
(pass) non-streaming: preserves response body when usage parsing fails [1.00ms]
(pass) non-streaming: preserves response.url routing metadata after body read [1.00ms]
(pass) non-streaming: strips <think> tag block from assistant content [1.00ms]
(pass) streaming: thinking block closed before tool call [2.00ms]
(pass) streaming: strips <think> tag block from assistant content deltas [1.00ms]
(pass) streaming: strips <think> tag split across multiple content chunks [1.00ms]
(pass) streaming: preserves prose without tags (no phrase-based false positive) [2.00ms]
(pass) strips credentials and query params from URL in fetch network error message [1.00ms]
(pass) classifies localhost transport failures with actionable category marker [2.00ms]
(pass) transport failures are not labeled with HTTP status 503 [1.00ms]
(pass) propagates AbortError without wrapping it as transport failure [1.00ms]
(pass) classifies chat-completions endpoint 404 failures with endpoint_not_found marker [1.00ms]
(pass) sel...
🧰 Additional context used
📓 Path-based instructions (5)
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
README.mdsrc/commands.tssrc/commands/login/login.tsxdocs/smart-routing.mdsrc/cost-tracker.tssrc/commands/smartroute/index.test.tssrc/commands/smartroute/index.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
README.mdsrc/commands.tssrc/commands/login/login.tsxdocs/smart-routing.mdsrc/cost-tracker.tssrc/commands/smartroute/index.test.tssrc/commands/smartroute/index.ts
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}
⚙️ CodeRabbit configuration file
{README.md,CONTRIBUTING.md,docs/**,.github/pull_request_template.md}: Review docs for accuracy against current code behavior. Flag security or provider claims that overpromise, stale install commands, missing setup caveats, and instructions that could push users toward unsafe credential handling. Keep purely wording-level suggestions non-blocking.
Files:
README.mddocs/smart-routing.md
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use TypeScript with strict mode
Use ESM imports in TypeScript source files
Files:
src/commands.tssrc/commands/login/login.tsxsrc/cost-tracker.tssrc/commands/smartroute/index.test.tssrc/commands/smartroute/index.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/commands/smartroute/index.test.ts
🔇 Additional comments (7)
src/cost-tracker.ts (1)
12-13: LGTM!Also applies to: 88-90, 294-299
src/commands/login/login.tsx (1)
4-6: LGTM!src/commands/smartroute/index.ts (1)
46-51: LGTM!Also applies to: 79-82, 90-93, 95-102
src/commands/smartroute/index.test.ts (1)
1-121: LGTM!src/commands.ts (1)
170-170: LGTM!Also applies to: 282-282
docs/smart-routing.md (1)
1-58: LGTM!README.md (1)
219-219: LGTM!
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 (2)
src/commands/smartroute/index.test.ts (1)
79-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing coverage:
/smartroute onwith env-only configured roles.This test only covers the fully-unconfigured case. Given the status test at lines 68-77 proves
readSmartRoutingnormalizes env-backed roles, add a case whereOPENCLAUDE_SMART_ROUTING_SIMPLE/_STRONGare set via env (nosmartRoutingsettings block) and assert/smartroute onsucceeds — this would have caught the gate-check bug flagged insrc/commands/smartroute/index.ts.As per path instructions, "Review tests for meaningful coverage of the changed behavior... Block when risky runtime changes lack focused regression coverage."
🤖 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/commands/smartroute/index.test.ts` around lines 79 - 85, Add a regression test in the smartroute command suite to cover /smartroute on when roles come only from environment variables and there is no smartRouting settings block. Reuse the existing test helpers in index.test.ts and the readSmartRouting normalization behavior already verified by the status test to set OPENCLAUDE_SMART_ROUTING_SIMPLE and OPENCLAUDE_SMART_ROUTING_STRONG, then assert call('on', ctx) succeeds instead of returning the “Set both roles first” rejection. This should exercise the gate in src/commands/smartroute/index.ts and prove env-backed roles are accepted.Source: Path instructions
src/commands/smartroute/index.ts (1)
47-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
/smartrouteshould use the normalized config, not rawsettings.smartRouting
readSmartRouting()only falls back toOPENCLAUDE_SMART_ROUTING*when the settings block is absent, but this command still gates and persists from the raw block. That makes/smartroute onreject valid env-only configs, and/smartroute simple|strongcan write a partial block that shadows the env defaults and disables routing. Use the normalized values here and persist the resolved role fields.🤖 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/commands/smartroute/index.ts` around lines 47 - 90, The /smartroute command is using the raw settings.smartRouting block instead of the normalized result from readSmartRouting(), which breaks env-only defaults and can shadow them with partial writes. Update the logic in the SmartRouting command handler to base validation and the enabled state on the normalized config returned by readSmartRouting(settings), and make persist/updateSettingsForSource write the resolved role fields rather than a partial smartRouting object. Ensure the /smartroute on, simple, and strong paths all read from and persist the same normalized SmartRoutingSettings shape.
🤖 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 `@src/commands/smartroute/index.test.ts`:
- Around line 79-85: Add a regression test in the smartroute command suite to
cover /smartroute on when roles come only from environment variables and there
is no smartRouting settings block. Reuse the existing test helpers in
index.test.ts and the readSmartRouting normalization behavior already verified
by the status test to set OPENCLAUDE_SMART_ROUTING_SIMPLE and
OPENCLAUDE_SMART_ROUTING_STRONG, then assert call('on', ctx) succeeds instead of
returning the “Set both roles first” rejection. This should exercise the gate in
src/commands/smartroute/index.ts and prove env-backed roles are accepted.
In `@src/commands/smartroute/index.ts`:
- Around line 47-90: The /smartroute command is using the raw
settings.smartRouting block instead of the normalized result from
readSmartRouting(), which breaks env-only defaults and can shadow them with
partial writes. Update the logic in the SmartRouting command handler to base
validation and the enabled state on the normalized config returned by
readSmartRouting(settings), and make persist/updateSettingsForSource write the
resolved role fields rather than a partial smartRouting object. Ensure the
/smartroute on, simple, and strong paths all read from and persist the same
normalized SmartRoutingSettings shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dab1fdd4-f3a2-4473-b617-f4b275a44157
📒 Files selected for processing (4)
src/commands/smartroute/index.test.tssrc/commands/smartroute/index.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/index.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use TypeScript with strict mode
Use ESM imports in TypeScript source files
Files:
src/services/api/smartRouting/index.test.tssrc/commands/smartroute/index.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/api/smartRouting/index.test.tssrc/commands/smartroute/index.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/smartRouting/index.test.tssrc/commands/smartroute/index.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/index.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/smartRouting/index.test.tssrc/commands/smartroute/index.test.ts
🔇 Additional comments (4)
src/services/api/smartRouting/index.ts (1)
7-7: LGTM!Also applies to: 20-31, 217-225
src/services/api/smartRouting/index.test.ts (1)
8-8: LGTM!Also applies to: 330-360
src/commands/smartroute/index.ts (1)
92-96: LGTM!src/commands/smartroute/index.test.ts (1)
1-60: LGTM!Also applies to: 86-154
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/smartroute/index.ts (1)
92-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor:
persist+formatPersistErrorpattern repeated 3x.
on,off, andsimple/strongeach repeatconst error = persist(next); if (error) return text(formatPersistError(error)). Not blocking, but a tiny helper would keep this from drifting if error handling changes later.♻️ Optional consolidation
+ const persistOrError = (next: SmartRoutingSettings): { error: string } | { next: SmartRoutingSettings } => { + const error = persist(next) + return error ? { error: formatPersistError(error) } : { next } + }Then in each branch:
const r = persistOrError(next); if ('error' in r) return text(r.error).Also applies to: 100-102, 115-117
🤖 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/commands/smartroute/index.ts` around lines 92 - 96, The persist-and-format error handling is duplicated across the on, off, and simple/strong branches in the smartroute command flow. Extract the repeated `persist(next)` plus `formatPersistError` check into a small shared helper in `index.ts` (for example, around the existing branch handlers) and have each branch call that helper so future error handling changes stay consistent.
🤖 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/services/api/smartRouting/index.test.ts`:
- Around line 119-122: The test for latestUserMessageHasNonTextContent is not
covering the meta/tool-result skip path because userMsg('plain') is the first
real match in the backward scan. Update the fixture in the smartRouting index
test so a non-text user message appears before the meta and tool-result carriers
in the array order, then assert true to verify the scan skips those carriers and
finds the earlier real message.
---
Outside diff comments:
In `@src/commands/smartroute/index.ts`:
- Around line 92-96: The persist-and-format error handling is duplicated across
the on, off, and simple/strong branches in the smartroute command flow. Extract
the repeated `persist(next)` plus `formatPersistError` check into a small shared
helper in `index.ts` (for example, around the existing branch handlers) and have
each branch call that helper so future error handling changes stay consistent.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9a1701e3-70a6-4ab2-b7a6-025efe52fb95
📒 Files selected for processing (6)
src/commands/smartroute/index.test.tssrc/commands/smartroute/index.tssrc/query.tssrc/services/api/smartModelRouting.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/index.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: PR Checks / smoke-and-tests: feat: smart auto-routing (per-turn simple-vs-strong model selection)
Conclusion: failure
st.ts:
(pass) getSchemaValidationErrorOverride > returns actionable missing-skill error for SkillTool
(pass) getSchemaValidationErrorOverride > does not override unrelated tool schema failures
(pass) getSchemaValidationErrorOverride > does not override SkillTool when skill is present
(pass) getSchemaValidationErrorOverride > uses the actionable override for structured toolUseResult too
(pass) getReplayModifiedFiles > captures file-editing tool paths
(pass) getReplayModifiedFiles > captures Bash simulated sed edit paths [1.00ms]
(pass) replay tool lifecycle records > records permission denied completions
(pass) replay tool lifecycle records > records success completions with modified files
(pass) replay tool lifecycle records > records error completions
(pass) replay tool lifecycle records > classifies abort-shaped tool failures as cancelled
(pass) replay tool lifecycle records > captures the final executable input
(pass) replay tool lifecycle records > normalizes denied file-tool replay inputs to match allowed retry inputs
(pass) replay tool lifecycle records > records one error terminal status when post-call result processing fails [2.00ms]
(pass) query lifecycle tool-use cleanup > successful tool execution leaves no active lifecycle tool use [1.00ms]
(pass) query lifecycle tool-use cleanup > custom validation failure leaves no active lifecycle tool use
(pass) query lifecycle tool-use cleanup > schema validation failure does not end a lifecycle entry that never started [1.00ms]
(pass) query lifecycle tool-use cleanup > unknown tool does not end a lifecycle entry that never started [1.00ms]
(pass) query lifecycle tool-use cleanup > permission denial leaves no active lifecycle tool use
(pass) query lifecycle tool-use cleanup > already-aborted tool execution uses reason-aware result for query-timeout
(pass) query lifecycle tool-use cleanup > already-aborted tool execution uses reason-aware result for hard_max
(pass) query lifecycle tool-use cle...
GitHub Actions: PR Checks / 2_smoke-and-tests.txt: feat: smart auto-routing (per-turn simple-vs-strong model selection)
Conclusion: failure
oes not normalize incomplete streamed Bash commands when finish_reason is length [1.00ms]
(pass) repairs truncated JSON objects even without command field [2.00ms]
(pass) preserves raw input for unknown plain string tool arguments [1.00ms]
(pass) preserves parsed string input for unknown JSON string tool arguments [1.00ms]
(pass) sanitizes malformed MCP tool schemas before sending them to OpenAI [2.00ms]
(pass) optional tool properties are not added to required[] — fixes Groq/Azure 400 tool_use_failed [1.00ms]
(pass) coalesces consecutive user messages to avoid alternation errors (issue `#202`) [1.00ms]
(pass) coalesces consecutive assistant messages preserving tool_calls (issue `#202`) [2.00ms]
(pass) non-streaming: reasoning_content emitted as thinking block only when content is null [1.00ms]
(pass) non-streaming: empty string content does not fall through to reasoning_content as text [1.00ms]
(pass) non-streaming: real content takes precedence over reasoning_content [1.00ms]
(pass) non-streaming: preserves response body when usage parsing fails [2.00ms]
(pass) non-streaming: preserves response.url routing metadata after body read [1.00ms]
(pass) non-streaming: strips <think> tag block from assistant content [1.00ms]
(pass) streaming: thinking block closed before tool call [1.00ms]
(pass) streaming: strips <think> tag block from assistant content deltas [2.00ms]
(pass) streaming: strips <think> tag split across multiple content chunks [1.00ms]
(pass) streaming: preserves prose without tags (no phrase-based false positive) [1.00ms]
(pass) strips credentials and query params from URL in fetch network error message [2.00ms]
(pass) classifies localhost transport failures with actionable category marker [2.00ms]
(pass) transport failures are not labeled with HTTP status 503 [2.00ms]
(pass) propagates AbortError without wrapping it as transport failure [1.00ms]
(pass) classifies chat-completions endpoint 404 failures with endpoint_not_found marker [...
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use TypeScript with strict mode
Use ESM imports in TypeScript source files
Files:
src/services/api/smartModelRouting.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.test.tssrc/query.tssrc/commands/smartroute/index.tssrc/services/api/smartRouting/index.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/services/api/smartModelRouting.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.test.tssrc/query.tssrc/commands/smartroute/index.tssrc/services/api/smartRouting/index.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/smartModelRouting.tssrc/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.test.tssrc/query.tssrc/commands/smartroute/index.tssrc/services/api/smartRouting/index.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/smartModelRouting.tssrc/services/api/smartRouting/index.test.tssrc/services/api/smartRouting/index.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/commands/smartroute/index.test.tssrc/services/api/smartRouting/index.test.ts
🔇 Additional comments (13)
src/services/api/smartRouting/index.ts (4)
104-136: LGTM!
275-286: LGTM!
65-87: 🩺 Stability & AvailabilityNo eviction bug here.
markSessionDisabled()clears the entire set once it reachesMAX_DISABLED_SESSIONS, so the map is capped and won't grow unbounded. The tradeoff is coarse rotation: all tracked session disables are dropped at once.> Likely an incorrect or invalid review comment.
138-153: 🩺 Stability & AvailabilityNo change needed: unclassified routed errors are intentionally retryable.
src/query.tsuses this only for the simple→strong smart-routing fallback, and the tests already cover 5xx, network, and undefined-status errors as retryable.> Likely an incorrect or invalid review comment.src/services/api/smartRouting/index.test.ts (1)
108-118: LGTM!Also applies to: 170-178
src/commands/smartroute/index.ts (1)
45-121: LGTM! Persist-then-check-error ordering and env-backed status/current resolution correctly address the prior review findings.src/commands/smartroute/index.test.ts (1)
86-101: LGTM!Also applies to: 126-135
src/query.ts (5)
138-151: LGTM!
1017-1017: 🎯 Functional CorrectnessNo type gap here.
messagesForQueryis already compatible withreadonly TurnCountMessage[], so this call is fine as-is.> Likely an incorrect or invalid review comment.
1568-1574: 🩺 Stability & AvailabilityNo provider-id refresh needed here.
pinnedRouteProviderIdis the anchor for the provider the route was first pinned against; keeping it unchanged letsshouldDropPinForProviderSwapstill drop the pin only if the active provider changes later.> Likely an incorrect or invalid review comment.
1598-1600: 📐 Maintainability & Code QualityNeed the inferred type here
Need the inferred type of
messagesForQueryfromgetMessagesAfterCompactBoundary(...)before deciding whether this double cast is hiding a real mismatch or is just redundant.
1550-1606: 🩺 Stability & AvailabilityDrop this concern —
isRetryableRoutedModelErroralready excludes 400/401/403, so the routed fallback won’t retry auth/permission/bad-request failures.> Likely an incorrect or invalid review comment.src/services/api/smartModelRouting.ts (1)
38-56: LGTM!Also applies to: 151-158
3 participants
Summary
src/services/api/smartModelRouting.tsinto the agent loop./smartroutecommand, asmartRoutingsettings block, orOPENCLAUDE_SMART_ROUTING*env vars.Impact
/smartroutecommand andsmartRoutingsetting. When enabled, simple turns go to the cheaper model;/costgains a routing summary (simple/strong split, simple->strong escalations, pricing-gated savings estimate). When disabled, the loop is byte-for-byte unchanged.transition === undefined); the routed-error fallback reuses the existingattemptWithFallbackretry loop rather than adding a new one; the org model allowlist is enforced by callingisModelAlloweddirectly; thinking-block stripping on a model change goes through the existing provider gate (preserve-reasoning providers like DeepSeek/GLM are left untouched). A mid-turn provider-fallback swap drops the pin so a stale model id is never sent to the new endpoint.resolveModelOnlyModelandstripThinkingBlocksIfProviderAllowsare newly exported (internal use).Testing
bun run buildbun run smokebun run check— passes except 5 pre-existing full-suite failures (getAgentModel alias + applyConfigEnvironmentVariables) that also fail onmainand are unrelated to this change; the routing tests are greensrc/services/api/smartRouting/*,src/commands/smartroute/*,conversationRecoveryprovider-gate,agentRouting,cost-tracker— all greenNotes
/smartroutestatus,/costsummary).query.tsis large; the inline routing/fallback blocks could be extracted into helpers. Deferred here rather than refactor the delicate generator inside this PR.Summary by CodeRabbit
Release Notes
/smartroutecommand to check status, enable/disable routing, and select role model keys./costto include a smart-routing usage summary; switching accounts now clears routing tallies too.