Repository navigation
feat: add hybrid model router - #194
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds policy-driven local and cloud model routing. The change adds configuration contracts, provider classification, privacy-based selection, redaction, timeout handling, fallback execution, settings controls, documentation, and tests. ChangesModel routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AIService
participant RoutedAIProvider
participant resolveModelRoute
participant ConfiguredProvider
AIService->>RoutedAIProvider: Create autoRouter provider
RoutedAIProvider->>resolveModelRoute: Resolve request route
resolveModelRoute-->>RoutedAIProvider: Return primary and fallback routes
RoutedAIProvider->>ConfiguredProvider: Execute primary chat with timeout
ConfiguredProvider-->>RoutedAIProvider: Return response or timeout/error
RoutedAIProvider->>ConfiguredProvider: Execute redacted fallback request
ConfiguredProvider-->>RoutedAIProvider: Return fallback response
RoutedAIProvider-->>AIService: Return response with routing metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/api/contracts.ts (1)
96-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve
defaultPrivacyLevelliteral suggestions.The current union collapses to
string, so IDE autocomplete won’t surface the known privacy levels. Use the TypeScriptstring & {}pattern while keeping the type open-ended.♻️ Proposed fix to preserve literal autocomplete
- defaultPrivacyLevel: 'public' | 'internal' | 'workspace-private' | 'repo-private' | 'secret' | string; + defaultPrivacyLevel: 'public' | 'internal' | 'workspace-private' | 'repo-private' | 'secret' | (sring & {});🤖 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/api/contracts.ts` at line 96, Update the defaultPrivacyLevel type in the relevant contract to intersect string with an empty object type, preserving the listed privacy-level literals for IDE suggestions while still allowing arbitrary string values.
🤖 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 `@node-backend/lib/ai.mjs`:
- Around line 66-73: Guard the `autoRouter`/`auto_router` provider path in
`RoutedAIProvider` so `modelRouter.localProvider`, `modelRouter.cloudProvider`,
or `settings.fallbackProvider` cannot resolve to another router instance. Update
the router settings resolution used by `resolveModelRoute` to reject or replace
these self-referential values with the existing safe fallback behavior before
the `createProvider` callback invokes `AIService.createProvider`.
In `@node-backend/lib/model-router.mjs`:
- Around line 62-75: The routerSettings fields enabled and logDecisions are
currently ignored. Wire routerSettings().enabled into the routing path so
disabled model routing is bypassed, and use routerSettings().logDecisions to
conditionally emit routing decision logs; otherwise remove both fields from the
configuration surface and documentation.
- Around line 85-87: Update redactModelRequestForCloud to redact every sensitive
pattern recognized by isSensitiveText, specifically raw ssh-rsa key blobs and
bare “private key” text without requiring a colon or equals separator. Ensure
cloudRedacted requests cannot retain the detected key material while preserving
existing redaction behavior for the other sensitive patterns.
- Around line 228-240: Update checkAuthentication to honor the routing mode used
by resolveModelRoute: for localOnly, check only config.localProvider; for
cloudOnly, check only config.cloudProvider; otherwise retain checking the
configured provider candidates. Preserve the existing authentication failure
handling and return true only when an eligible provider authenticates.
- Around line 145-147: Update the performanceFirst decision branch to set
timeoutMs based on the task type: use config.backgroundTimeoutMs for background
tasks and config.localTimeoutMs for normal tasks, instead of returning null.
Preserve the existing cloud-primary and local-fallback routing in this branch.
- Around line 207-214: Update the RoutedAIProvider constructor to initialize
config with the routing-specific values localProvider, cloudProvider, and mode
from settings, so createProvider’s checkAuthentication cache key distinguishes
each route while preserving the existing providerType behavior.
In `@node-backend/tests/model-router.test.mjs`:
- Around line 1-19: Set the HOME and PROJECTS_DIR environment variables at the
beginning of the model-router test module, before any tests or imported test
logic run, using isolated test-safe paths consistent with the repository’s
existing test convention.
---
Nitpick comments:
In `@src/api/contracts.ts`:
- Line 96: Update the defaultPrivacyLevel type in the relevant contract to
intersect string with an empty object type, preserving the listed privacy-level
literals for IDE suggestions while still allowing arbitrary string values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dfdc2e5e-74cb-4dae-aab4-c5077353ee84
📒 Files selected for processing (6)
docs/model-routing.mdnode-backend/lib/ai.mjsnode-backend/lib/model-router.mjsnode-backend/lib/orchestrator.mjsnode-backend/tests/model-router.test.mjssrc/api/contracts.ts
…vider routing, and redactions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
node-backend/tests/model-router.test.mjs (1)
7-12: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winInitialize the test environment before loading
model-router.mjs.ESM evaluates the static import on Line 12 before Lines 8-10 run.
HOMEandPROJECTS_DIRtherefore do not protect module initialization that reads these variables.fs.mkdtempSync()also violates the backend async-I/O rule.Create the directory with
await fs.promises.mkdtemp(). Then dynamically importmodel-router.mjsafter setting both variables.Proposed fix
-import { resolveModelRoute, redactModelRequestForCloud, RoutedAIProvider } from '../lib/model-router.mjs'; - // Set HOME and PROJECTS_DIR before test imports/executions per project conventions -const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'model-router-test-')); +const tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'model-router-test-')); process.env.HOME = tmpDir; process.env.PROJECTS_DIR = path.join(tmpDir, 'projects'); + +const { resolveModelRoute, redactModelRequestForCloud, RoutedAIProvider } = + await import('../lib/model-router.mjs');As per coding guidelines, “Set
HOMEandPROJECTS_DIRin tests to prevent access to real user data” and “Use async/await for all I/O in the Node.js backend.”🤖 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 `@node-backend/tests/model-router.test.mjs` around lines 7 - 12, Update the test setup before importing model-router.mjs: replace synchronous fs.mkdtempSync with awaited fs.promises.mkdtemp, set HOME and PROJECTS_DIR, then dynamically import and destructure resolveModelRoute, redactModelRequestForCloud, and RoutedAIProvider. Ensure no static import loads model-router.mjs before the environment variables are initialized.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 `@node-backend/tests/model-router.test.mjs`:
- Around line 71-85: Update the performance-first route test around
resolveModelRoute so a cloud-primary route does not expose the localTimeoutMs
value as route.timeoutMs. Return null for timeoutMs in this scenario, or use a
distinct cloud timeout setting and assert that value instead; preserve the
openAiCli primary and ollama fallback assertions.
---
Duplicate comments:
In `@node-backend/tests/model-router.test.mjs`:
- Around line 7-12: Update the test setup before importing model-router.mjs:
replace synchronous fs.mkdtempSync with awaited fs.promises.mkdtemp, set HOME
and PROJECTS_DIR, then dynamically import and destructure resolveModelRoute,
redactModelRequestForCloud, and RoutedAIProvider. Ensure no static import loads
model-router.mjs before the environment variables are initialized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 278e0701-1380-4d38-a7be-b1ae1b137caf
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
node-backend/lib/ai.mjsnode-backend/lib/model-router.mjsnode-backend/tests/model-router.test.mjssrc/api/contracts.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- node-backend/lib/ai.mjs
- src/api/contracts.ts
- node-backend/lib/model-router.mjs
…vider interface - Implement full Model Routing UI in ModelSettings.tsx with Routing Mode, Fallback Action, and Provider selectors - Add explanatory banner clarifying behavior when preferred models are unavailable or unreachable - Refactor AIProvider base class and subclasses to implement isCloudProvider() - Add Provider Location toggle for Custom CLIs in ProviderSettings.tsx - Add unit tests for isCloudProvider across built-in and custom providers
…to single source of truth - Delete hardcoded CLOUD_PROVIDER_IDS and LOCAL_PROVIDER_IDS sets from model-router.mjs - Export single isCloudProviderId helper from base.mjs - Delegate AIService.isCloudProvider and model-router functions to isCloudProviderId
AssafMiron
previously approved these changes
Aug 4, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@node-backend/lib/providers/base.mjs`:
- Around line 32-51: The isCloudProviderId function in
node-backend/lib/providers/base.mjs (lines 32-51) and
CustomCliProvider.isCloudProvider method in
node-backend/lib/providers/custom.mjs (lines 86-96) both duplicate the same
isCloud-boolean-then-id heuristic, including a loose .includes('local')
substring check that can incorrectly mark cloud-hosted custom CLI configurations
as local if their id or name contains the word "local". Create a shared helper
function that consolidates the local-provider detection logic and replace the
substring check with exact-match logic (such as checking for the 'custom-local-'
prefix and exact 'local' token match) or support an explicit isLocal flag on the
custom CLI object. Update both isCloudProviderId and
CustomCliProvider.isCloudProvider to call this shared helper instead of
duplicating the heuristic.
In `@src/components/settings/ModelSettings.tsx`:
- Around line 166-174: Provide programmatic labels for all routing controls: in
src/components/settings/ModelSettings.tsx at 166-174 link the routing-enable
Switch to a purpose-identifying Label; at 194-216 and 219-250 link each
SelectTrigger to its Label; at 253-296 assign unique IDs to both provider
selectors and matching label references; at 298-324 assign unique IDs to both
timeout inputs and matching label references. In
src/components/settings/ProviderSettings.tsx at 585-599 link the
provider-location Switch to its Label.
- Around line 304-318: The onChange handlers for both the localTimeoutMs and
backgroundTimeoutMs Input elements use parseInt with a fallback operator that
incorrectly accepts negative values (since -1 is truthy) and rejects 0 (since 0
is falsy). Replace the parseInt-or-fallback logic with proper validation that
accepts only finite non-negative integer values, explicitly handling the 0 case.
Additionally, add matching min and step HTML attributes to both Input elements
to enforce the valid range at the input level.
- Around line 102-117: Normalize legacy custom CLI entries to a persisted
canonical isCloud value before building provider lists in ModelSettings.tsx,
using the backend’s ID-based local detection when the field is absent. Update
ProviderSettings.tsx so changing a custom provider’s location also clears or
moves saved modelRouter.localProvider and modelRouter.cloudProvider values when
their provider role no longer matches, and persist the normalized location.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09b03bc4-5bd3-4447-ac2c-668473a46ce8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
node-backend/lib/ai.mjsnode-backend/lib/model-router.mjsnode-backend/lib/providers/base.mjsnode-backend/lib/providers/custom.mjsnode-backend/lib/providers/ollama.mjsnode-backend/tests/model-router.test.mjssrc/api/contracts.tssrc/components/settings/ModelSettings.tsxsrc/components/settings/ProviderSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- node-backend/lib/model-router.mjs
- node-backend/lib/ai.mjs
- src/api/contracts.ts
…d accessibility controls - Consolidate local/cloud custom CLI detection into shared isCloudCustomCli helper in base.mjs - Update CustomCliProvider and isCloudProviderId to use isCloudCustomCli exact-match logic - Ensure model-router.test.mjs uses async mkdtemp and sets process.env prior to dynamic import - Fix performance-first cloud route to return null for local timeout threshold - Add programmatic htmlFor and unique IDs to all routing controls in ModelSettings.tsx and ProviderSettings.tsx - Validate local and background timeout input ranges and support zero timeout values - Sync saved modelRouter local and cloud provider selections when custom CLI location changes
AssafMiron
approved these changes
Aug 4, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
node-backend/lib/providers/base.mjs (1)
139-142: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOverride provider classification or remove the default cloud flag.
AIProvider.isCloudProvider()defaults totrue, andmodel-router.mjsuses that method directly instead of the provider-ID fallback.ClaudeCodeProvider,GoogleCliProvider,HostedAPIProvider, andOpenAiCliProviderinherit the default, so routing metadata reports them as cloud. Add the missing local/provider overrides, or remove the inherited default while preserving the ID-based fallback for new providers.🤖 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 `@node-backend/lib/providers/base.mjs` around lines 139 - 142, Correct the provider classification defined by AIProvider.isCloudProvider(): add explicit false overrides in ClaudeCodeProvider, GoogleCliProvider, HostedAPIProvider, and OpenAiCliProvider for local providers, or remove the default true implementation while retaining model-router.mjs’s provider-ID fallback for unclassified providers.
🤖 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 `@node-backend/lib/providers/base.mjs`:
- Around line 139-142: Correct the provider classification defined by
AIProvider.isCloudProvider(): add explicit false overrides in
ClaudeCodeProvider, GoogleCliProvider, HostedAPIProvider, and OpenAiCliProvider
for local providers, or remove the default true implementation while retaining
model-router.mjs’s provider-ID fallback for unclassified providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e2a5ba87-448a-496e-abda-10c4d780beec
📒 Files selected for processing (6)
node-backend/lib/model-router.mjsnode-backend/lib/providers/base.mjsnode-backend/lib/providers/custom.mjsnode-backend/tests/model-router.test.mjssrc/components/settings/ModelSettings.tsxsrc/components/settings/ProviderSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- node-backend/lib/providers/custom.mjs
- node-backend/tests/model-router.test.mjs
- src/components/settings/ProviderSettings.tsx
- src/components/settings/ModelSettings.tsx
- node-backend/lib/model-router.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the missing backend implementation for ProductOS model routing via the existing
autoRouterprovider type. ProductOS can now route between a local trained provider, currently Ollama by default, and a cloud/fallback provider using explicit privacy/performance policy.Specification
Provider activation
Set
activeProvidertoautoRouter.{ "activeProvider": "autoRouter", "modelRouter": { "enabled": true, "mode": "auto", "localProvider": "ollama", "cloudProvider": "hostedApi", "fallback": "cloudRedacted", "localTimeoutMs": 3000, "backgroundTimeoutMs": 15000, "defaultPrivacyLevel": "workspace-private", "logDecisions": true } }Routing modes
auto: prefer local for private/workspace data; public requests can fallback to cloud.privacyFirst: prefer local even when privacy metadata is missing.performanceFirst: cloud-first for public requests, local fallback; private data remains local-first.localOnly: always local, no fallback.cloudOnly: always cloud, no fallback.Fallback modes
none: fail fast, no fallback.cloud: send original request to cloud.cloudRedacted: redact obvious secrets/tokens/private-key blocks before cloud fallback.askUser: fail with approval-required message instead of sending private data to cloud.local: local fallback for cloud-first routes.Latency behavior
localTimeoutMs, default3000ms.backgroundTimeoutMs, default15000ms.Routing metadata
Responses include
metadata.routingwith:Implementation notes
node-backend/lib/model-router.mjswith deterministic routing policy, cloud-redaction helper, andRoutedAIProvider.autoRouterintoAIService.createProviderand backend provider support checks.ModelRouterConfigto the frontend API contracts.docs/model-routing.mdwith configuration, policy, latency, metadata, and safety notes.Safety
cloudRedactedis a backstop, not a full DLP system. Highly sensitive work should uselocalOnlyoraskUseruntil stronger classification/redaction exists.Verification
node --test node-backend/tests/model-router.test.mjs— 5/5 passingnpm run build— passingnpm run test:backend— 148/148 passingSummary by CodeRabbit
New Features
Documentation