feat(agent-routing): assign a per-agent model from the /agents menu#1632
feat(agent-routing): assign a per-agent model from the /agents menu#1632beardthelion wants to merge 21 commits into
Conversation
…etail view Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (15)src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/components/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,py,json,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**⚙️ CodeRabbit configuration file
Files:
{src/services/**/*.ts,src/utils/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.test.{ts,tsx,js}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/integrations/**/*.ts,src/services/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (8)
📝 WalkthroughWalkthroughAdds per-agent model route configuration and model-only route resolution. A new ChangesPer-agent model route configuration and model-only resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 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/components/agents/AgentRouteSelector.tsx`:
- Around line 35-37: The route option discovery at line 35 uses
getInitialSettings() which may include settings from non-user scopes, while the
persistence calls to setAgentRoute at lines 46-47 and 57 write to user-settings
scope. This scope mismatch can create shadow entries in user-settings for keys
that only exist in non-user scopes, altering routing behavior. Ensure consistent
scope usage by either using a user-settings-specific getter instead of
getInitialSettings() when building options in buildRouteOptions at line 35, or
by adjusting how setAgentRoute at lines 46-47 and 57 reads and persists settings
to match the same scope used for option discovery.
In `@src/services/api/agentRouteSettings.test.ts`:
- Around line 112-115: The test named 'returns the route key, or undefined for
none/dangling' in the currentRouteValue test block does not actually test the
dangling case, creating a mismatch between the test name and its assertions.
Either add an additional expect statement that tests the dangling case (with
appropriate kind value) to match the test name description, or rename the test
to accurately reflect that it only tests the 'model-only' and 'none' cases and
remove the reference to dangling.
In `@src/services/api/agentRouteSettings.ts`:
- Around line 31-33: The cross-provider detection logic is inconsistent between
two locations in the file. The condition at line 31 treats a key as
cross-provider when either base_url or api_key exists (using OR logic:
entry.base_url || entry.api_key), but the condition at lines 94-98 apparently
applies different logic (requiring both to exist). Align these two detection
points by applying the same condition in both places—ensure that the same
criteria determine whether a route is cross-provider in both the initial read
and the option-label paths so the UI presentation remains 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 Plus
Run ID: 325c2f3c-1a47-47fb-993b-0cd60eaff94b
📒 Files selected for processing (4)
src/components/agents/AgentDetail.tsxsrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/components/agents/AgentRouteSelector.tsxsrc/components/agents/AgentDetail.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/components/agents/AgentRouteSelector.tsxsrc/components/agents/AgentDetail.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/components/agents/AgentRouteSelector.tsxsrc/components/agents/AgentDetail.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/components/agents/AgentRouteSelector.tsxsrc/components/agents/AgentDetail.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/agentRouteSettings.test.ts
🔇 Additional comments (1)
src/components/agents/AgentDetail.tsx (1)
32-38: LGTM!Also applies to: 228-231
…-provider labels
Build route options from getSettingsForSource('userSettings') instead of
getInitialSettings(), matching the scope we persist to. Reading from the
merged view could surface an agentModels key that only exists in a
non-user scope; selecting it would write a shadow user-settings entry that
diverges from the original (losing its base_url/api_key).
Label a key as cross-provider when either base_url or api_key is present,
matching readAgentRoute, so the same route is described consistently.
Also assert the dangling case in the currentRouteValue test, which the
old test name referenced but did not cover.
|
All three addressed in 6ddd129.
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that need to be addressed before this is ready.
Findings
-
[P2] Commit custom model ids only after input submit
src/components/agents/AgentRouteSelector.tsx:45
The custom-model option saves from the input option'sonChange, butCustomSelectcalls that handler on every text edit and paste. As soon as a user focuses "Enter a custom model id" and types the first character ofgpt-5-mini, this callssetAgentRoute(agentType, "g")and closes the selector, so the advertised custom-id path cannot actually enter a full model id. The later submit path only calls the parentonChangewithCUSTOM_MODEL_VALUE, which this component ignores. Please commit the typed value from the input submit path instead of persisting from the per-keystroke input handler. -
[P2] Match route display/selection to runtime credential validation
src/services/api/agentRouteSettings.ts:31
readAgentRouteandbuildRouteOptionstreat anagentModelsentry as cross-provider when eitherbase_urlorapi_keyexists, but the runtime resolver requires both and skips partial entries. For a user-settings entry like{ "half-entry": { "base_url": "https://api.example.com/v1" } },/agentsnow shows/selects it as a cross-provider route and writesagentRoutingto it, while the spawned agent falls back becausetoAgentRoutereturnsnull. Please use the same validity rule as runtime here, or surface the entry as unconfigured so the UI does not claim a route that will not execute.
…red rule The custom-model input persisted from the per-keystroke onChange, so typing the first character of a model id saved that single character and closed the selector. Track the typed value and persist only on submit (the sentinel fires the Select onChange with the full value). Cross-provider detection now mirrors the runtime resolver (toAgentRoute): a route is cross-provider only when both base_url and api_key are present. A partial entry is skipped at runtime and inherits, so readAgentRoute reports it as unconfigured (dangling) and buildRouteOptions labels it 'unconfigured, inherits' rather than claiming a cross-provider route that will not execute.
|
Both addressed in 20f718f.
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Match the picker to normalized/default runtime routes
src/services/api/agentRouteSettings.ts:26
The new UI reads and writesagentRoutingwith an exactagentTypekey, but the runtime resolver normalizes route keys case-insensitively, treats hyphen and underscore as equivalent, and falls back todefault. For example, an existing user setting like{ "agentRouting": { "general_purpose": "mini" } }routesgeneral-purposeagents at runtime, but/agentsnow reports that agent as inheriting. If the user then selects another model, the UI writes a newgeneral-purposeentry beside the existinggeneral_purposeentry; runtime keeps the first normalized entry and still runs the old model while the UI shows the new one. The same display gap applies to agents currently routed bydefault. Please have the picker read, update, and clear the same effective route key thatresolveAgentProviderwould use, or otherwise surface these normalized/default routes so the menu cannot claim a route change that runtime ignores.
…e route key The runtime resolver normalizes routing keys (case-insensitive, hyphen/underscore-equivalent) and falls back to default, but the picker read and wrote agentRouting with the exact agentType key. So an existing general_purpose route showed general-purpose agents as inheriting, and selecting a model wrote a general-purpose sibling that first-wins lookup ignored while the menu claimed the change took effect. readAgentRoute now matches the normalized per-agent key the resolver would use, and surfaces a default-fallback route with viaDefault. computeSetRouteUpdate/computeClearRouteUpdate overwrite or clear that existing key spelling instead of writing a sibling. The clear option is hidden for default-inherited routes since there is no own key to remove.
|
Fixed in 7aba642. The picker now resolves against the same effective route key as the runtime resolver.
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Do not save routes that higher-priority settings will ignore
src/services/api/agentRouteSettings.ts:185
The new picker reads and writes onlyuserSettings, but the runtime route resolver runs from the merged settings chain, where project, local, and policy settings are merged after user settings and therefore win. If a repo already has.openclaude/settings.jsonwith{ "agentRouting": { "verification": "project-mini" } },/agentsnow reportsverificationas inheriting becausegetAgentRoute()only reads the user file. When the user pressesmand selectsgpt-5-mini,setAgentRoute()writes{ "verification": "gpt-5-mini" }to user settings, closes successfully, and the UI can now show the user route, but the next verifier spawn still resolves the project route fromgetInitialSettings(). Please make the picker surface/handle the effective higher-priority route, or refuse/explain edits that cannot take effect instead of saving a lower-priority route that runtime will ignore.
…ides The runtime resolver reads merged settings (userSettings -> projectSettings -> localSettings -> flagSettings -> policySettings), but the picker read and wrote only userSettings. A project/local/policy agentRouting entry made /agents report the agent as inheriting, and selecting a model wrote a userSettings route the merged chain ignored while the menu reported success. getAgentRoute now reads the effective merged settings, so the route line reflects what runtime resolves. setAgentRoute/clearAgentRoute refuse with an explanation when a higher-priority source owns the normalized route key, and the picker surfaces that as a read-only notice instead of offering an edit that cannot take effect.
|
Fixed in fedf141. The picker no longer saves routes the merged settings chain would ignore.
Added |
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/components/agents/AgentRouteSelector.tsx (1)
24-109:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd focused regression tests for selector interaction paths before merge.
This component now owns critical user-visible routing behavior (shadow read-only mode, custom-ID submit semantics, clear/set write paths, error display), but the PR only adds service-helper tests. Please add focused tests for these selector flows so regressions in UI-driven persistence are caught.
As per coding guidelines, “Add or update tests when the change affects behavior in TypeScript and JavaScript files” and “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/components/agents/AgentRouteSelector.tsx` around lines 24 - 109, Add focused regression tests for the AgentRouteSelector component to cover its critical user-visible routing behaviors. Create test cases that verify: the shadow read-only mode display and behavior when getRouteShadowSource returns a value, custom model ID input and submission flow via the CUSTOM_MODEL_VALUE path, clear and set operations through the onChange handler with different value parameters (CLEAR_ROUTE_VALUE and regular values), and error display when persistence operations fail. These tests should exercise the apply function's error handling, the customIdRef tracking mechanism for custom IDs, and the onClose callback invocation in various scenarios to ensure UI-driven persistence works correctly across all interaction paths.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api/agentRouteSettings.ts`:
- Around line 252-256: The shadowError function generates a generic error
message that assumes all setting sources are file-backed, but flagSettings has
no file to edit. Modify the shadowError function to branch the error message
based on the source parameter: when source is flagSettings, provide guidance
pointing to flags or profile configuration instead of telling users to edit a
settings file; for other sources, keep the current file-editing guidance. This
ensures the remediation advice is appropriate for each source type.
---
Outside diff comments:
In `@src/components/agents/AgentRouteSelector.tsx`:
- Around line 24-109: Add focused regression tests for the AgentRouteSelector
component to cover its critical user-visible routing behaviors. Create test
cases that verify: the shadow read-only mode display and behavior when
getRouteShadowSource returns a value, custom model ID input and submission flow
via the CUSTOM_MODEL_VALUE path, clear and set operations through the onChange
handler with different value parameters (CLEAR_ROUTE_VALUE and regular values),
and error display when persistence operations fail. These tests should exercise
the apply function's error handling, the customIdRef tracking mechanism for
custom IDs, and the onClose callback invocation in various scenarios to ensure
UI-driven persistence works correctly across all interaction paths.
🪄 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: 0f1b99af-e1dc-45a9-b31d-757492139bef
📒 Files selected for processing (3)
src/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/services/api/agentRouteSettings.test.tssrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/services/api/agentRouteSettings.test.tssrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.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/agentRouteSettings.test.tssrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.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/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/agentRouteSettings.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/api/agentRouteSettings.test.tssrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.ts
🔇 Additional comments (1)
src/services/api/agentRouteSettings.test.ts (1)
1-262: LGTM!
flagSettings has no file to edit (it comes from the --settings flag or SDK inline settings), so the shadow message no longer tells users to edit a nonexistent file. Extracted shadowRemediation so the error and the read-only selector notice share one source-aware string. Added focused AgentRouteSelector tests covering the shadow read-only mode (file-backed and flag sources), persisting a selected model, and surfacing a failed save without closing the dialog.
|
Both addressed in 975fde2.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/agents/AgentRouteSelector.test.tsx`:
- Around line 79-91: The test "persists a selected model" is not deterministic
because it selects a route by ordinal index ('1') without controlling what
options are available, and it only asserts the agentType in the setCalls rather
than the complete call tuple. Fix this by ensuring the getRouteShadowSource mock
returns a controlled set of route options with known model keys at known
positions, and then update the assertion on setCalls to verify the complete
tuple [agentType, modelKey] rather than just the agentType alone. This ensures
the test actually proves the correct model key was selected and persisted.
🪄 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: b6d152d1-8736-45b7-85d0-b695c1ca2671
📒 Files selected for processing (4)
src/components/agents/AgentRouteSelector.test.tsxsrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,py,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Follow the existing code style in the touched files
Files:
src/components/agents/AgentRouteSelector.test.tsxsrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add or update tests when the change affects behavior in TypeScript and JavaScript files
Files:
src/components/agents/AgentRouteSelector.test.tsxsrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/components/agents/AgentRouteSelector.test.tsxsrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/components/agents/AgentRouteSelector.test.tsxsrc/services/api/agentRouteSettings.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/components/agents/AgentRouteSelector.test.tsxsrc/components/agents/AgentRouteSelector.tsxsrc/services/api/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.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/agentRouteSettings.test.tssrc/services/api/agentRouteSettings.ts
🔇 Additional comments (15)
src/services/api/agentRouteSettings.ts (8)
252-261: LGTM!
1-51: LGTM!
53-76: LGTM!
78-114: LGTM!
116-154: LGTM!
156-209: LGTM!
211-250: LGTM!
263-268: LGTM!src/services/api/agentRouteSettings.test.ts (6)
1-29: LGTM!
30-108: LGTM!
110-159: LGTM!
161-222: LGTM!
224-262: LGTM!
264-275: LGTM!src/components/agents/AgentRouteSelector.tsx (1)
12-12: LGTM!Also applies to: 52-52
The persist test selected an option by ordinal without controlling the option list and only asserted the agentType. Mock buildRouteOptions to a known single option and assert the full [agentType, modelKey] tuple, so the test proves the selected model key is what gets persisted.
|
Fixed in 8cc54ec. The set-route test now mocks |
bun's mock.module persists across files in the same process and mock.restore() does not undo it, so mocking buildRouteOptions in the selector test leaked into agentRouteSettings.test.ts and failed its buildRouteOptions assertions in the full suite (passed in isolation). Re-point the module at the real implementation in afterEach, matching the AgentsMenu.test.tsx pattern.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Keep rejecting model keys shadowed by higher settings
src/services/api/agentRouteSettings.ts:326
The new shadow guard still skips the check wheneveruserSettings.agentModels[modelKey]already exists, but that does not make the saved route take effect. Settings are merged from user to project/local/flag/policy, andresolveAgentProvider()later readsagentModels[modelKey]from the merged settings, so a project or policyagentModels.sonnetstill wins over the user'sagentModels.sonnet. In that case the picker even labels the option as shadowed, but selecting it writesuserSettings.agentRouting.verification = "sonnet"successfully and the next spawn resolves to the higher-priority provider/model instead of the user's current-provider route. Please keep the model-key shadow rejection whenever a higher-priority source defines the same key, or write user-created routes under a collision-proof key before allowing the save.
… owns them The previous shadow guard skipped the check when userSettings already defined agentModels[modelKey], but that does not make the route take effect: agentModels merges by source priority, so a higher-priority project/policy entry for the same key still wins and the agent resolves to that provider/model. The picker even flagged the key as shadowed while the save path let it through, so offer and save disagreed. Drop the ownsKey carve-out so setAgentRoute rejects whenever a higher source defines the key. Extract collectShadowedModelKeys as the single definition of 'defined above userSettings' that both the offer flag (getShadowedModelKeys) and the save guard (findModelKeyShadowingSource) derive from, and add a test locking the two paths in agreement.
|
Fixed in bd10cdf. You're right, the Dropped the carve-out, so |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found a couple of test coverage gaps around the fixes from the last review cycle.
Findings
-
[P3] Complete CodeRabbit's shadow-write guard test request
src/services/api/agentRouteSettings.ts:337
CodeRabbit's review item asked for focused coverage of the actualsetAgentRoute()model-key shadow guard, including the higher-priority-shadow rejection path. The current patch covers the pure helpers (findModelKeyShadowingSource()/collectShadowedModelKeys()) and the option labeling, but it still never callssetAgentRoute()with a higher-priorityagentModels[modelKey]source. That means the user-visible save guard could be dropped or bypassed later while these tests continue to pass, reintroducing the route-save bug where the picker accepts a key that resolves to project/policy settings instead of the user's current-provider model. Please complete that request with directsetAgentRoute()coverage for both the rejected shadowed-key case and the allowed user-only-key case. -
[P3] Add a spawn-level regression for model-only teammate routes
src/tools/AgentTool/AgentTool.tsx:342
The model-only teammate fix is currently covered at the helper level byresolveOutOfProcessTeammateModelOnly(), but the actualAgentToolteammate-spawn tests still only assert cross-provideragentModelsrouting at thespawnTeammate()boundary. A future change could stop passingroutedTeammateModelOnlyintospawnTeammate()and the helper tests would still pass, even though pane/window teammates would go back to inheriting the parent model instead of using the route saved from/agents. Please add anAgentTool.teammateModelregression with a model-only route such asagentModels.mini = { model: "gpt-5-mini" }andagentRouting.verification = "mini", then assert the spawned teammate receivesmodel: "gpt-5-mini"with no cross-provider override.
…teammate spawn Adds direct setAgentRoute() coverage for the higher-priority-shadow rejection and the allowed user-only-key save, and an AgentTool teammate-spawn regression that a model-only route reaches spawnTeammate as the resolved model with no cross-provider override.
|
Both added in 76c9a44. setAgentRoute() now has direct coverage for the shadowed-key rejection (asserting no write lands) and the user-only-key save; AgentTool has a model-only teammate-spawn regression (agentModels.mini = { model: "gpt-5-mini" }, agentRouting.verification = "mini") asserting the spawn receives model gpt-5-mini with no cross-provider override. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for grinding through the previous review items. The latest commit covers the two specific asks I had on the shadow-write guard and model-only teammate spawn boundary, and the focused tests/build/typecheck/smoke checks are green locally.
One remaining coverage gap still worries me because it is exactly the interaction that regressed earlier in this PR:
-
[P3] Add a regression for custom model input submission
src/components/agents/AgentRouteSelector.test.tsxexercises selecting a built-in model and failed-save behavior, but none of the component tests focus the custom-model input option, type or paste a custom id, submit it, and assert thatsetAgentRoute(agentType, 'gpt-5-mini')receives the full id. That custom input path is the one that previously saved only the first character because the inputonChangepath was treated like a committed selection. The current implementation now appears to route typed input throughcustomIdRefand commit on submit, but without an interaction regression this can break again while the existing tests still pass. Please add a focusedAgentRouteSelectortest that enters a custom id and verifies the full id is persisted on submit.
|
Added a focused |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for grinding through the previous review items. I took another pass through the updated routing, settings, and teammate-spawn paths and found one issue that still needs to be addressed.
Findings
- [P2] Make the custom model input regression pass on Windows
src/components/agents/AgentRouteSelector.test.tsx:271
The latest regression test for custom model submission is still failing in a fresh Windows checkout. After selecting the custom input option, the rendered frame shows the focused row as> 4. e.g. gpt-5-mini, but the test waits for a line containing the Unicode❯glyph, so it times out before typing or submitting the custom id. This means the new coverage for the exact custom-input bug is not actually green locally, even though the component path may be fixed. Please make the wait condition assert focus/input state without depending on that glyph, or accept both rendered cursor forms, sobun test src/components/agents/AgentRouteSelector.test.tsxpasses and continues to protect the full-id submission path.
The custom model regression test waited for the ❯ focus pointer, but ink renders figures.pointer as ">" on Windows, so the wait timed out in a Windows checkout before typing the id. Match figures.pointer directly so the wait tracks whatever glyph the renderer emits on each platform.
|
Fixed in 11e3a7a. The wait now matches |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for grinding through the previous review items. I found one remaining issue in the new /agents route picker interaction.
Findings
-
[P2] Keep Esc scoped to the route picker while it is open
src/components/agents/AgentDetail.tsx:66AgentDetailkeeps its parentconfirm:nohandler active after switching into routing mode, even though the nestedAgentRouteSelectoralso passesonCancel={onClose}to itsSelect. Because the parent handler was registered first and resolves only theConfirmationcontext, a bare Esc is consumed byonBackbefore the child select can resolveescapetoselect:cancel. I reproduced this with anAgentDetail+KeybindingSetuprender: pressmto open the route picker, then press Esc;onBackis invoked instead of just returning to the agent detail view. In the full/agentsdialog this sends the user out of the selected-agent detail rather than merely closing the model picker, despite the picker/read-only copy telling users that Esc goes back. Please make the parent detail/dialog cancel handlers inactive whileroutingis true, or otherwise ensure the route selector owns Esc, and add an AgentDetail-level regression formthen Esc.
AgentDetail kept its parent confirm:no (Esc -> onBack) handler active after switching into routing mode. Since the Confirmation context was registered first, a bare Esc resolved to confirm:no and exited the detail view instead of resolving to the nested select's select:cancel, despite the picker copy telling users Esc goes back. Gate confirm:no with isActive=!routing so the route selector owns Esc and a single Esc only closes the picker. Add an AgentDetail regression covering m then Esc.
|
Fixed in ceb3e78. The detail-level |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the route-picker interaction and teammate routing paths and found a couple of issues that still need to be addressed.
Findings
-
[P2] Complete the Esc fix for the enclosing AgentsMenu dialog
src/components/agents/AgentsMenu.tsx:522
The latest patch disablesAgentDetail's ownconfirm:nohandler while the route picker is open, but the real/agentsview still wraps that detail component in aDialogwhoseonCancelremains active.Dialogregisters its ownconfirm:nohandler by default, andAgentDetailhas no way to tell the wrapper thatroutingis true. The new regression mountsAgentDetaildirectly, so it misses this outer handler. In the full menu, pressingmopensAgentRouteSelector, whoseSelectexpects Esc to callonClose, but the enclosing dialog can still consume Esc and jump back to the agent menu instead of simply closing the picker. Please complete the previous Esc-scoping request by making the enclosingDialogcancel inactive while the route picker is open, or by otherwise letting the selector own Esc in the fullAgentsMenupath, and cover that full wrapper interaction in the regression. -
[P2] Preserve cross-provider routes in in-process teammate mode
src/tools/AgentTool/AgentTool.tsx:367
The teammate path now resolves/agentscross-provider routes before callingspawnTeammate(), but it only passesroutedTeammateProvider.modelplusmodelWasToolSpecified: false. That is enough for pane/window teammates because the child CLI can re-resolve--modelagainst settings and apply the provider override, but in-process teammates do not cross the CLI startup path.handleSpawnInProcess()forwards only the model string, andrunInProcessTeammate()builds a synthetic agent definition from that string; nobaseURL/apiKeyprovider override or original route key is preserved. For common configs where the route key differs from the API model, for exampleagentRouting.verification = "ds"andagentModels.ds = { model: "deepseek-chat", base_url, api_key }, the in-process teammate runsdeepseek-chaton the parent provider instead of the configured DeepSeek/OpenAI-compatible provider. OpenClaude uses in-process teammates when that mode is selected or when pane backend detection falls back, so saved/agentscross-provider routes still fail in a supported teammate mode. Please carry the provider override (or enough route identity to re-resolve it) through the in-process handoff and add coverage past theAgentTool->spawnTeammate()mock boundary.
… teammate routes Two follow-ups from review of the per-agent model routing UI. The /agents detail view wraps AgentDetail in a Dialog whose own confirm:no (onCancel) stayed active while the route picker was open. As the first-registered Confirmation context it swallowed Esc and dropped back to the agent list instead of just closing the picker. AgentDetail now reports its picker state up through onRoutingChange, and a small AgentDetailDialog wrapper feeds that into the Dialog's isCancelActive so the picker's Select owns Esc while it is open. In-process teammates run the same runAgent() as normal subagents, but the synthetic agent definition overwrites agentType with the teammate's display name, so the original subagent_type that agentRouting is keyed on was lost and the configured cross-provider route never resolved (the teammate ran the routed model on the parent provider). The original subagent_type is now carried through the in-process handoff and used as the routing key, matching what pane and window teammates already get by re-resolving from their CLI identity. Adds a regression test for the picker Esc interaction through the Dialog wrapper and resolver tests proving the route resolves from the original subagent_type rather than the teammate name.
|
Both addressed in e2ea45b. Esc scoping for the enclosing Dialog. The detail view now reports its route-picker state up via an In-process teammate routes. Rather than carry a pre-resolved override, I carry the route identity: the original |
2 participants
What and why
Per-agent model routing already works through settings (
agentModels+agentRouting), but the only way to set it was hand-editing~/.openclaude.json, and the built-in agents' route keys (verification,Explore,Plan) were undiscoverable. This adds a way to do it from the/agentsmenu.Open any agent in
/agents, pressm, and pick a model. You can choose a model on your current provider (typing a custom id for OpenAI-compatible providers), point at an already-defined cross-provider entry, or clear the route to remove the agent's own assignment (it then falls back to thedefaultroute if one is configured, otherwise inherits the main-loop model). It writes to user-global settings and takes effect on the next spawn of that agent type. Built-in agents are routable the same way, since routing is keyed by agent type.Impact
/agentsdetail view (pressm).agentModels/agentRoutingunder user settings.sonnet/opus/haiku/inherit) now resolve throughgetAgentModelwith the active permission mode, so on non-Claude-native providers they inherit the parent model instead of sending a literal alias that 404s. Real model ids still pass through unchanged. The same model-only resolution now also applies to pane/window teammate spawns, which previously honored only cross-provider routes and otherwise inherited the parent.--setting-sourcesexcluding them): the picker reports an error instead of saving a route to a file the runtime would never load.Design notes
The settings-shaping logic lives in a pure module (
agentRouteSettings.ts) with thin I/O wrappers, unit-tested with plain objects. The selector is a small standalone component. The route resolver (agentRouting.ts) was extended so model-only alias routes go throughgetAgentModeland receivepermissionMode, threaded fromAgentTool/runAgent; the cross-provider override path is unchanged. The touch to the (React-Compiler-output)AgentDetailis minimal and does not alter existing memo slots. Creating new cross-provider credentials from the TUI is intentionally out of scope; those stay JSON-managed.Checks
bun test src/services/api/agentRouting.test.ts— 54 passbun test src/services/api/agentRouteSettings.test.ts src/services/api/agentRouting.test.ts src/utils/settings/— passbun test src/tools/AgentTool/— 46 passbun run typecheck— cleanbun run build && bun run smoke— pass (0.18.0)bun test— only pre-existing sandbox-only failures (3/export, 1/lsp recommend, 1usagechild-process error) that pass on CI; unrelated to this change.Provider path
The model-only resolution that this now routes through
getAgentModelis provider-aware: built-in aliases inherit the parent on non-Claude-native providers, and OpenAI-compatible custom model ids pass through as-is. Cross-provider routes (fullagentModelsentries withbase_url+api_key) are unchanged and still apply across providers.UI
This adds terminal UI. From
/agents, opening an agent shows aRoute:line and(press "m" to change); pressingmopens a select with the model options plus a custom-id input and a clear option. Happy to add an asciinema/screenshot if useful.Summary by CodeRabbit
New Features
permissionModeduring agent runs and teammate spawning.Bug Fixes
Tests