feat: support per-request model override in /v1/chat/completions - #103
Conversation
- add optional model override to completion request types\n- forward request model through gateway, worker, and orchestrator proxy paths\n- use request model in NEAR AI providers with fallback to active model\n- replace model-mismatch integration test with override propagation checks\n- update FEATURE_PARITY.md note for OpenAI-compatible API behavior\n\nRefs nearai#49
Summary of ChangesHello @raahimSal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of the OpenAI-compatible API by introducing support for per-request model overrides. Previously, the API enforced a strict match between the requested model and the active model, limiting dynamic model selection. This change allows users to specify a different model for each completion request, improving adaptability for various use cases without mutating shared provider state. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements per-request model overrides for the OpenAI-compatible API, which is a great feature enhancement. However, it introduces a potential Denial of Service (DoS) vulnerability in the streaming path due to repeated allocations of the user-supplied model name in SSE chunks, which should be addressed by validating or truncating the model name. The suggested refactorings for duplicated code have been deferred to a follow-up task, aligning with the PR's primary goal of feature enhancement. Additionally, there are suggestions for improving the flexibility of with_model functions.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Clean implementation of per-request model override. The approach is correct:
- Remove the hard model-mismatch rejection in the OpenAI-compat handler
- Thread an
Option<String>model field throughCompletionRequest/ToolCompletionRequest - Propagate through proxy structs in worker/orchestrator layers
- Providers fall back to active model when the override is None
Test coverage is solid -- the renamed test_chat_completions_model_override now verifies the override flows through to the mock provider via MockLlmState. The Arc<MockLlmState> pattern for tracking received models is clean.
One minor suggestion for a future follow-up: consider logging when a model override is used (at debug level) for observability, since silent model switching could be confusing during debugging.
Closes #49. Good to merge.
|
Follow-up (previous comment had shell formatting issues):
Latest head commit: @zmanian could you take a quick re-review on the latest commit when you have a moment? |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request successfully implements per-request model overrides for the OpenAI-compatible /v1/chat/completions endpoint. The changes are logically sound, propagating the model option through various layers from the web handler down to the LLM providers. The test suite has been effectively updated to cover the new functionality, including model override success cases and input validation failure cases.
My review includes one suggestion to refactor some duplicated code in src/channels/web/openai_compat.rs to improve long-term maintainability. Overall, this is a well-executed feature addition.
There was a problem hiding this comment.
Code Review
This pull request successfully implements per-request model overrides for the OpenAI-compatible API, which is a great feature enhancement. The changes are well-structured, propagating the model override through various layers of the application, from the web handler to the LLM providers. The accompanying tests are thorough and validate the new functionality effectively. I have one minor suggestion to improve efficiency by avoiding an unnecessary string clone in the streaming path.
ilblackdragon
left a comment
ilblackdragon
left a comment
There was a problem hiding this comment.
Code Review: Per-request model override in /v1/chat/completions
Overall this is a well-structured PR that cleanly threads the model override through the request pipeline without mutating shared state. The approach of adding Option<String> to the request types and falling back to the active model in the providers is the right design.
Issues
1. Duplicate with_llm_provider call in src/main.rs (low)
The base branch already sets the LLM provider on the gateway via the builder chain:
let mut gw = GatewayChannel::new(gw_config.clone()).with_llm_provider(llm.clone());The PR adds a second call:
gw = gw.with_llm_provider(Arc::clone(&llm));This is harmless (just overwrites the same value), but it is redundant and may confuse future readers who wonder whether the two calls serve different purposes. Remove the duplicate.
2. Model override silently ignored by other LLM providers (medium)
Only NearAiProvider and NearAiChatProvider were updated to read req.model. The other LlmProvider implementations -- OpenAiCompatibleChatProvider (src/llm/openai_compatible_chat.rs), RigAdapter (src/llm/rig_adapter.rs), and FailoverProvider (which delegates to inner providers) -- still unconditionally use self.active_model_name() and silently ignore the per-request model field.
If someone configures IronClaw with one of these providers and sends a model override, the override will be silently dropped with no warning. At minimum, consider:
- Adding
req.model.unwrap_or_else(|| self.active_model_name())toOpenAiCompatibleChatProviderandRigAdapteras well (same pattern used in the NEAR AI providers), or - Logging a warning in the
LlmProvidertrait's default behavior whenreq.modelisSomebut the provider doesn't handle it.
3. validate_model_name could be stricter on character set (low/nit)
The validation checks for empty and length > 256, which addresses the DoS concern raised in the earlier review. However, it does not restrict the character set. Model names containing control characters, newlines, or other non-printable characters could end up in JSON responses, log output, or SSE streams. Consider adding a check like:
if !model.bytes().all(|b| b.is_ascii_graphic() || b == b' ') {
return Err("model contains invalid characters".to_string());
}This is a defense-in-depth measure since serde_json will escape control chars in output, but it prevents odd values from reaching provider APIs.
Positive observations
- The
Option<String>field withunwrap_or_else(|| self.active_model_name())in providers is the correct zero-mutation approach. Internal callers that don't setmodelget the existing behavior automatically. - Worker/orchestrator proxy structs (
ProxyCompletionRequest,ProxyToolCompletionRequest) correctly propagate the field, maintaining the override across the container boundary. - Test coverage is solid: the
MockLlmStatepattern cleanly verifies the model reaches the provider, andtest_chat_completions_model_too_longvalidates the length guard. - The 256-byte limit on model names addresses the streaming DoS concern from the earlier review.
Summary
The core feature is correctly implemented. The main substantive concern is issue #2 (incomplete provider coverage), which could cause confusion when users expect model overrides to work with non-NEAR AI backends. Issue #1 is a quick cleanup. Issue #3 is a hardening suggestion.
Batch 2 PR Review: nearai/ironclaw PRs 111, 110, 109, 103, 95, 74Reviewer: AI Sub-Agent PR #111: Fix backwards compatibility for nearai.session_tokenSummaryThis PR adds backwards compatibility for the nearai session management by implementing a fallback mechanism. When Pros
Concerns
Suggestions
PR #110: Add env docs for local LLM providersSummaryThis PR updates the Pros
Concerns
Suggestions
PR #109: Normalize memory search query and update marked.jsSummaryThis PR addresses two security and stability issues in the web interface. First, it adds input normalization for memory search queries to prevent excessive query lengths and invalid input types. Second, it updates the marked.js dependency to a specific version with integrity hashing for supply chain security. The changes are defensive in nature, preventing potential DoS attacks and ensuring the integrity of third-party JavaScript dependencies. Pros
Concerns
Suggestions
PR #103: Per-request model override for OpenAI-compatible APISummaryThis is a significant feature PR that adds per-request model override capability across the entire LLM provider ecosystem. Previously, all requests used the active model, but now clients can specify a different model per request. The changes span multiple modules: request structs now include optional Pros
Concerns
Suggestions
PR #95: Add Venice AI provider and embeddingsSummaryThis PR adds comprehensive support for Venice AI as both an LLM provider and an embeddings provider. It introduces a new Pros
Concerns
Suggestions
PR #74: Security fix: Enhanced HTTP response size validationSummaryThis PR strengthens the HTTP tool's defense against OOM attacks by implementing two-stage response size validation. First, it checks the Pros
Concerns
Suggestions
Overall RecommendationsHigh Priority
Medium Priority
Low Priority
General Observations
|
There was a problem hiding this comment.
Pull request overview
Implements issue #49 by making model selection request-scoped for the OpenAI-compatible /v1/chat/completions endpoint, propagating an optional per-request model through internal request types and proxy layers without mutating shared provider state.
Changes:
- Add
model: Option<String>toCompletionRequestandToolCompletionRequest, with builder helpers. - Pass request model through OpenAI-compatible handler (non-streaming + streaming), NEAR AI providers, and worker/orchestrator proxy request structs.
- Update integration tests and feature parity documentation; add model name validation in the OpenAI-compatible handler.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/openai_compat_integration.rs | Extends mock provider/tests to assert per-request model propagation and adds model validation test cases. |
| src/channels/web/openai_compat.rs | Removes model mismatch rejection, validates model name, and forwards request model through streaming/non-streaming codepaths. |
| src/llm/provider.rs | Introduces optional per-request model on internal request types plus with_model() helpers. |
| src/llm/nearai.rs | Uses request model override (fallback to active model) when building NEAR AI requests (including retry path). |
| src/llm/nearai_chat.rs | Uses request model override (fallback to active model) when building NEAR AI chat-completions requests. |
| src/llm/rig_adapter.rs | Warns when an override is requested but ignored for providers that can’t switch models per request. |
| src/worker/api.rs | Preserves model through worker→orchestrator proxy request structs. |
| src/orchestrator/api.rs | Maps proxy request model into internal completion/tool-completion requests. |
| src/main.rs | Injects the shared LLM provider into the web gateway channel. |
| FEATURE_PARITY.md | Notes per-request model override support for the OpenAI-compatible API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let model_name = requested_model; | ||
| let id = chat_completion_id(); | ||
| let created = unix_timestamp(); | ||
|
|
||
| if has_tools { | ||
| let tools = convert_tools(req.tools.as_deref().unwrap_or(&[])); | ||
| let mut tool_req = ToolCompletionRequest::new(messages, tools); | ||
| let mut tool_req = ToolCompletionRequest::new(messages, tools).with_model(req.model); |
There was a problem hiding this comment.
The OpenAI response model is set from the requested model string (requested_model / req.model) even when the underlying provider ignores per-request overrides (e.g. RigAdapter explicitly warns that it uses the configured model). This can cause the API response to report a model that was not actually used. Consider plumbing the actual model used back from the provider (e.g. add model_used to CompletionResponse/ToolCompletionResponse, or add a provider capability/method to resolve the effective model) and use that for the OpenAI model field.
| @@ -584,7 +590,7 @@ async fn handle_streaming( | |||
|
|
|||
| let llm_result = if has_tools { | |||
| let tools = convert_tools(req.tools.as_deref().unwrap_or(&[])); | |||
| let mut tool_req = ToolCompletionRequest::new(messages, tools); | |||
| let mut tool_req = ToolCompletionRequest::new(messages, tools).with_model(req.model); | |||
There was a problem hiding this comment.
Same as the non-streaming path: model_name is taken from req.model for SSE chunks, which can misreport the model if the provider does not actually honor per-request overrides (e.g. RigAdapter ignores overrides). The streamed model field should reflect the effective model used by the provider.
| @@ -24,7 +24,21 @@ const AUTH_TOKEN: &str = "test-openai-token"; | |||
| // Mock LLM provider | |||
| // --------------------------------------------------------------------------- | |||
|
|
|||
| struct MockLlmProvider; | |||
| #[derive(Default)] | |||
| struct MockLlmState { | |||
| completion_models: Mutex<Vec<Option<String>>>, | |||
| tool_completion_models: Mutex<Vec<Option<String>>>, | |||
| } | |||
|
|
|||
| struct MockLlmProvider { | |||
| state: Arc<MockLlmState>, | |||
| } | |||
|
|
|||
| impl MockLlmProvider { | |||
| fn new(state: Arc<MockLlmState>) -> Self { | |||
| Self { state } | |||
| } | |||
| } | |||
|
|
|||
| #[async_trait] | |||
| impl LlmProvider for MockLlmProvider { | |||
| @@ -37,6 +51,12 @@ impl LlmProvider for MockLlmProvider { | |||
| } | |||
|
|
|||
| async fn complete(&self, req: CompletionRequest) -> Result<CompletionResponse, LlmError> { | |||
| self.state | |||
| .completion_models | |||
| .lock() | |||
| .expect("completion_models lock poisoned") | |||
| .push(req.model.clone()); | |||
|
|
|||
| // Echo the last user message back | |||
There was a problem hiding this comment.
This test uses std::sync::Mutex inside async LlmProvider methods. While it’s probably fine here because the lock isn’t held across .await, std::sync::Mutex can still block the Tokio runtime under contention. Using tokio::sync::Mutex (or parking_lot::Mutex) would avoid potential runtime blocking in async code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the review comments in follow-up commits.
Validation run:
Latest head: f883b8d. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if model.trim().is_empty() { | ||
| return Err("model must not be empty".to_string()); | ||
| } |
There was a problem hiding this comment.
The validation checks if model.trim().is_empty() but doesn't actually use the trimmed version. This means a model name like " gpt-4 " (with leading/trailing spaces) would pass validation and be sent to the provider with the spaces intact. Consider either rejecting strings that don't equal their trimmed version, or trimming the model name before validation and use.
| if model.trim().is_empty() { | |
| return Err("model must not be empty".to_string()); | |
| } | |
| let trimmed = model.trim(); | |
| if trimmed.is_empty() { | |
| return Err("model must not be empty".to_string()); | |
| } | |
| if trimmed != model { | |
| return Err("model must not have leading or trailing whitespace".to_string()); | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/llm/response_cache.rs:260
- Now that
CompletionRequestsupports per-requestmodel, the response cache key must incorporate the effective model for the request; otherwise prompts sent to different models can collide and return cached responses from the wrong model. Update the cache key computation to userequest.model(or better:inner.effective_model_name(request.model.as_deref())) and add/adjust tests here to assert differentmodeloverrides create different cache entries.
fn simple_request() -> CompletionRequest {
CompletionRequest {
messages: vec![ChatMessage::user("hello")],
model: None,
max_tokens: None,
temperature: None,
stop_sequences: None,
metadata: Default::default(),
}
}
fn different_request() -> CompletionRequest {
CompletionRequest {
messages: vec![ChatMessage::user("goodbye")],
model: None,
max_tokens: None,
temperature: None,
stop_sequences: None,
metadata: Default::default(),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Resolve which model should be reported for a given request. | ||
| /// | ||
| /// Providers that ignore per-request model overrides should override this | ||
| /// and return `active_model_name()`. | ||
| fn effective_model_name(&self, requested_model: Option<&str>) -> String { | ||
| requested_model | ||
| .map(std::borrow::ToOwned::to_owned) | ||
| .unwrap_or_else(|| self.active_model_name()) | ||
| } |
There was a problem hiding this comment.
effective_model_name() will often be called on wrapper providers (e.g. CircuitBreakerProvider/CachedProvider) rather than the underlying concrete provider. Those wrappers currently don’t override this method, so the default implementation will report requested_model even when the inner provider ignores overrides (e.g. RigAdapter). Add effective_model_name() implementations to wrapper providers that delegate to inner.effective_model_name(requested_model) so OpenAI responses report the model actually used.
| fn effective_model_name(&self, requested_model: Option<&str>) -> String { | ||
| self.providers[self.last_used.load(Ordering::Relaxed)].effective_model_name(requested_model) | ||
| } |
There was a problem hiding this comment.
FailoverProvider::effective_model_name() uses last_used (a shared atomic) to pick which inner provider to query. Under concurrent requests, last_used can be updated by another request between the completion call and model reporting, causing the OpenAI response to report the wrong model/provider. Consider returning the chosen provider index/model name from try_providers() along with the response (or otherwise threading the chosen provider through the request path) so model reporting is request-scoped.
|
🔴 High Severity — Response cache key ignores per-request model override
let key = cache_key(self.inner.model_name(), &request);The Scenario:
Impact: Wrong model responses served when response cache is enabled. Verified Suggested fix: // In CachedProvider::complete()
let model = request.model.as_deref().unwrap_or_else(|| self.inner.model_name());
let key = cache_key(model, &request);Also add a test in |
|
🟡 Medium Severity — Wrapper providers don't delegate
The default trait implementation returns the requested model if present: fn effective_model_name(&self, requested_model: Option<&str>) -> String {
requested_model.map(ToOwned::to_owned).unwrap_or_else(|| self.active_model_name())
}But fn effective_model_name(&self, _requested_model: Option<&str>) -> String {
self.active_model_name()
}Problem: When the provider chain is Suggested fix — add delegation in both wrappers: // In CachedProvider and CircuitBreakerProvider LlmProvider impls:
fn effective_model_name(&self, requested_model: Option<&str>) -> String {
self.inner.effective_model_name(requested_model)
}This ensures the override behavior of the innermost provider propagates through the wrapping chain. |
| e, | ||
| "invalid_request_error", | ||
| )); | ||
| } |
There was a problem hiding this comment.
🟡 Medium Severity — No model allowlist for cost control
The old code rejected requests where the model didn't match the configured model (404 model_not_found). This new validation only checks syntax (empty, length, control chars) but accepts any model name and forwards it to the upstream NEAR AI API.
An authenticated client could request expensive models (e.g., claude-opus-4-20250514) through the operator's NEAR AI credentials, bypassing the operator's intended model/cost budget.
Suggestion: Consider an optional allowed_models: Option<Vec<String>> config. If set, validate req.model against it before forwarding. If unset, allow any model (preserving current behavior). This gives operators opt-in cost control without breaking the feature for those who want open model routing.
This is a design consideration rather than a bug — the current behavior may be intentional for NEAR AI's usage model. Worth documenting the security implication either way.
|
Thanks everyone for the thorough review and detailed feedback. I pushed a follow-up commit (
Validation run on this branch:
All green locally. For the optional model allowlist / cost-control suggestion: I agree it is useful, but it broadens scope into policy/config. I kept this PR focused on correctness and compatibility fixes; happy to open a follow-up issue if maintainers want that in a separate change. |
…rai#103) * feat: support per-request model override for /v1/chat/completions - add optional model override to completion request types\n- forward request model through gateway, worker, and orchestrator proxy paths\n- use request model in NEAR AI providers with fallback to active model\n- replace model-mismatch integration test with override propagation checks\n- update FEATURE_PARITY.md note for OpenAI-compatible API behavior\n\nRefs nearai#49 * Wire gateway OpenAI-compatible routes to active LLM provider * Validate OpenAI model name length before streaming * Address PR103 review feedback on model override and validation * Report effective model in OpenAI-compatible responses * Use async mutexes in OpenAI compatibility integration tests * fix tests for per-request model field in response cache * fix formatting and clippy lint after main merge * Fix model override reporting and cache correctness --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
…rai#103) * feat: support per-request model override for /v1/chat/completions - add optional model override to completion request types\n- forward request model through gateway, worker, and orchestrator proxy paths\n- use request model in NEAR AI providers with fallback to active model\n- replace model-mismatch integration test with override propagation checks\n- update FEATURE_PARITY.md note for OpenAI-compatible API behavior\n\nRefs nearai#49 * Wire gateway OpenAI-compatible routes to active LLM provider * Validate OpenAI model name length before streaming * Address PR103 review feedback on model override and validation * Report effective model in OpenAI-compatible responses * Use async mutexes in OpenAI compatibility integration tests * fix tests for per-request model field in response cache * fix formatting and clippy lint after main merge * Fix model override reporting and cache correctness --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Summary
Why
PR #31 introduced the OpenAI-compatible API and explicitly tracked per-request model routing as follow-up in #49. This PR closes that gap without mutating shared provider state.
Test plan
All passes succeeded (11/11 each run).
Closes #49
Refs #31