Support model and effort selection for external agents#1125
Support model and effort selection for external agents#1125gptme-thomas wants to merge 2 commits into
Conversation
Greptile SummaryThis PR adds first-class model and effort selection for external-agent providers, extending
Confidence Score: 4/5The change is well-structured and backed by targeted tests; the core bind/select/forward flow works correctly for the common case. The has_model_arg/has_effort_arg guards in both runtimes miss the equals-sign form (--model=value), meaning users who configure base args that way receive a duplicate conflicting flag silently. The external_agent_model_id logic is also duplicated across two modules with the prefix string hardcoded in one of them, which is a latent divergence risk. Neither issue affects the main happy path exercised by the tests, but both represent real gaps for production configurations. crates/external-agents/src/runtimes/claude_code.rs and crates/external-agents/src/runtimes/codex.rs (has_model_arg / has_effort_arg); crates/gateway/src/external_agents.rs (duplicate external_agent_model_id).
|
| Filename | Overview |
|---|---|
| crates/gateway/src/external_agents.rs | Core session and bind logic extended with model/effort persistence; duplicates external_agent_model_id already defined in control_handlers.rs using a hardcoded prefix string instead of the shared constant. |
| crates/gateway/src/channel_events/commands/control_handlers.rs | Adds external-agent entries to /model, defines EXTERNAL_AGENT_MODEL_PREFIX constant and external_agent_model_id — logic is then duplicated in external_agents.rs without sharing the constant. |
| crates/external-agents/src/runtimes/claude_code.rs | Forwards --model and --effort to Claude Code CLI with duplicate-detection guards; guards miss the --flag=value (equals-sign) form that users might put in base args. |
| crates/external-agents/src/runtimes/codex.rs | Inserts --model and -c model_reasoning_effort before the app-server subcommand; has_model_arg also misses --model=value form. |
| crates/external-agents/src/types.rs | Adds model and effort optional fields to ExternalAgentSpec; straightforward and correct. |
| crates/config/src/schema.rs | Adds models and efforts Vec fields with serde default; straightforward and correct. |
| docs/src/configuration-reference.md | Adds external_agents and external_agents.agents. config reference sections; accurate and complete. |
Sequence Diagram
sequenceDiagram
participant User
participant ControlHandler as control_handlers.rs
participant ExtAgentSvc as GatewayExternalAgentService
participant Metadata as SqliteSessionMetadata
participant Runtime as ClaudeCode/Codex Runtime
User->>ControlHandler: "/model <number> (external-agent entry)"
ControlHandler->>ExtAgentSvc: "bind({ sessionKey, kind, model, effort })"
ExtAgentSvc->>Metadata: set_external_agent(session_key, kind, None)
ExtAgentSvc->>Metadata: set_model(session_key, "external-agent::kind::model::effort")
ExtAgentSvc-->>ControlHandler: "{ ok, modelId }"
ControlHandler-->>User: Backend switched to: ...
User->>ControlHandler: send message
ControlHandler->>ExtAgentSvc: session_for_binding(session_key, kind)
ExtAgentSvc->>Metadata: get(session_key)
Metadata-->>ExtAgentSvc: "{ model: "external-agent::kind::model::effort" }"
ExtAgentSvc->>ExtAgentSvc: selected_external_agent(model_id, kind)
ExtAgentSvc->>Runtime: "start_session(spec { model, effort })"
Runtime->>Runtime: args_for_turn() → [..., --model, model, --effort, effort]
Runtime-->>ExtAgentSvc: response text
ExtAgentSvc-->>User: broadcast + optional channel reply
Comments Outside Diff (1)
-
crates/gateway/src/external_agents.rs, line 855-869 (link)Duplicated
external_agent_model_idimplementation across modulesexternal_agent_model_idis defined here inexternal_agents.rsand again incontrol_handlers.rswith the same logic but a different signature (&strvsAgentTransportKind). The prefix"external-agent::"is hardcoded in the format strings here while a named constantEXTERNAL_AGENT_MODEL_PREFIX = "external-agent::"already exists incontrol_handlers.rs. CLAUDE.md explicitly requires: "DRY cross-crate types: define once in the lower-level crate, re-export from the higher-level one. Never duplicate." A divergence between the two format functions (e.g., someone updating one but not the other) would silently break parsing across the model-selection flow.Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Support external agent model and effort ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn has_model_arg(args: &[String]) -> bool { | ||
| args.iter() | ||
| .any(|arg| matches!(arg.as_str(), "--model" | "-m")) | ||
| } | ||
|
|
||
| fn has_effort_arg(args: &[String]) -> bool { | ||
| args.iter().any(|arg| arg == "--effort") | ||
| } |
There was a problem hiding this comment.
has_model_arg and has_effort_arg miss the --flag=value form
If a user configures args = ["--model=opus"] or args = ["--effort=low"] in their agent config (equals-sign form), these guards return false and a second conflicting flag is appended — e.g. ["--model=opus", "--model", "claude-opus-4"]. The codex version handles --model=value via strip_prefix("--config=") but the Claude Code version has no equivalent.
| fn has_model_arg(args: &[String]) -> bool { | |
| args.iter() | |
| .any(|arg| matches!(arg.as_str(), "--model" | "-m")) | |
| } | |
| fn has_effort_arg(args: &[String]) -> bool { | |
| args.iter().any(|arg| arg == "--effort") | |
| } | |
| fn has_model_arg(args: &[String]) -> bool { | |
| args.iter().any(|arg| { | |
| matches!(arg.as_str(), "--model" | "-m") | |
| || arg.starts_with("--model=") | |
| || arg.starts_with("-m=") | |
| }) | |
| } | |
| fn has_effort_arg(args: &[String]) -> bool { | |
| args.iter() | |
| .any(|arg| arg == "--effort" || arg.starts_with("--effort=")) | |
| } |
| async fn unbind_external_agent_if_bound( | ||
| state: &GatewayState, | ||
| session_key: &str, | ||
| ) -> ChannelResult<()> { | ||
| let status = state | ||
| .services | ||
| .external_agent | ||
| .status(serde_json::json!({ "sessionKey": session_key })) | ||
| .await | ||
| .map_err(ChannelError::unavailable)?; | ||
| if status | ||
| .get("bound") | ||
| .and_then(|value| value.as_bool()) | ||
| .unwrap_or(false) | ||
| { | ||
| state | ||
| .services | ||
| .external_agent | ||
| .unbind(serde_json::json!({ "sessionKey": session_key })) | ||
| .await | ||
| .map_err(ChannelError::unavailable)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| struct ExternalAgentModelSelection<'a> { | ||
| kind: &'a str, | ||
| model: Option<&'a str>, | ||
| effort: Option<&'a str>, |
There was a problem hiding this comment.
efforts-only config silently produces no selectable entries
When a user configures only efforts = ["high", "xhigh"] (without models), external_agent_model_entries generates only the bare default entry — the for model in models loop never executes. Yet external_agent_model_id already encodes the (None, Some(effort)) case as external-agent::{kind}::default::{effort}, so the persistence layer is ready to store effort-only selections. A user who sets just efforts expecting to pick a reasoning level for the default model will see no choices.
| if matches!(arg.as_str(), "--model" | "-m") { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
has_model_arg misses --model=value form in base args
Similar to the claude_code.rs issue: if a user puts --model=gpt-5.5 as a single element in their args config, the guard returns false and a second --model flag is injected before app-server. The equals-sign form is not covered by the matches!(arg.as_str(), "--model" | "-m") branch.
| if matches!(arg.as_str(), "--model" | "-m") { | |
| return true; | |
| } | |
| if matches!(arg.as_str(), "--model" | "-m") | |
| || arg.starts_with("--model=") | |
| || arg.starts_with("-m=") | |
| { | |
| return true; | |
| } |
- DRY: consolidate `external_agent_model_id`, `parse_external_agent_model_id`, and `EXTERNAL_AGENT_MODEL_PREFIX` into `external_agents.rs`; remove duplicates from `control_handlers.rs` which now imports via `crate::external_agents`. - Fix `has_model_arg` in both claude_code and codex runtimes to detect `--model=value` (equals-sign) form, preventing duplicate conflicting flags. - Fix `has_effort_arg` in claude_code runtime to detect `--effort=value` form. - Rewrite `selected_external_agent` to delegate to `parse_external_agent_model_id`. - Add 18 tests covering model ID round-tripping, arg detection, and kind filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Adds first-class model and effort selection for external-agent providers in
/model.The implementation covers:
models = [...]andefforts = [...]config for external agents/model, grouped underexternal-agent/<kind>--modeland--effortforwarding--modelandmodel_reasoning_effortforwardingValidation
cargo fmt --all -- --checkcargo test -p moltis-config external_agent -- --nocapturecargo test -p moltis-external-agents runtimes::claude_code::tests -- --nocapturecargo test -p moltis-external-agents runtimes::codex::tests -- --nocapturecargo test -p moltis-gateway external_agents -- --nocapture