fix(llm): fail fast on HTTP 400 invalid-model (stop the auto-model retry hang) - #5043
fix(llm): fail fast on HTTP 400 invalid-model (stop the auto-model retry hang)#5043abbyshekit wants to merge 2 commits into
Conversation
|
This PR was not deployed automatically as @abbyshekit does not have access to the Railway project. In order to get automatic PR deploys, please add @abbyshekit to your workspace on Railway. |
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a crate-visible ChangesInvalid model detection and non-retryable error mapping
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
size: M
![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 introduces logic to detect and handle permanent client errors caused by invalid or unknown model requests (HTTP 400) from the NearAI provider, mapping them to LlmError::ModelNotAvailable to prevent infinite retry loops. The review feedback suggests refining the error detection substring matching to avoid false positives with model parameter errors, and improving the robustness of the model name extraction function to correctly handle escaped quotes and colon separators.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub(crate) fn invalid_model_error(status_code: u16, response_text: &str) -> Option<String> { | ||
| if status_code != 400 { | ||
| return None; | ||
| } | ||
| let lower = response_text.to_ascii_lowercase(); | ||
| // Must be about the model, and say it is unusable. Guard against matching a | ||
| // context-length 400 (handled separately) by requiring model-name wording. | ||
| let mentions_model = lower.contains("model"); | ||
| let is_invalid = lower.contains("not found") | ||
| || lower.contains("not a valid model") | ||
| || lower.contains("invalid model") | ||
| || lower.contains("unknown model") | ||
| || lower.contains("is not a valid model name or alias"); | ||
| if !(mentions_model && is_invalid) { | ||
| return None; | ||
| } | ||
| Some(parse_invalid_model_name(response_text).unwrap_or_else(|| "unknown".to_string())) | ||
| } |
There was a problem hiding this comment.
Checking for "invalid model" or "unknown model" as substrings can lead to false positives when the error message is actually about a model parameter (e.g., "invalid model parameter: temperature must be between 0 and 2" or "unknown model parameter"). This would incorrectly map parameter validation errors to a non-retryable ModelNotAvailable error, preventing the user from seeing the actual parameter error. We should explicitly exclude these parameter-related phrases to ensure correct error propagation.
| pub(crate) fn invalid_model_error(status_code: u16, response_text: &str) -> Option<String> { | |
| if status_code != 400 { | |
| return None; | |
| } | |
| let lower = response_text.to_ascii_lowercase(); | |
| // Must be about the model, and say it is unusable. Guard against matching a | |
| // context-length 400 (handled separately) by requiring model-name wording. | |
| let mentions_model = lower.contains("model"); | |
| let is_invalid = lower.contains("not found") | |
| || lower.contains("not a valid model") | |
| || lower.contains("invalid model") | |
| || lower.contains("unknown model") | |
| || lower.contains("is not a valid model name or alias"); | |
| if !(mentions_model && is_invalid) { | |
| return None; | |
| } | |
| Some(parse_invalid_model_name(response_text).unwrap_or_else(|| "unknown".to_string())) | |
| } | |
| pub(crate) fn invalid_model_error(status_code: u16, response_text: &str) -> Option<String> { | |
| if status_code != 400 { | |
| return None; | |
| } | |
| let lower = response_text.to_ascii_lowercase(); | |
| // Must be about the model, and say it is unusable. Guard against matching a | |
| // context-length 400 (handled separately) by requiring model-name wording. | |
| let mentions_model = lower.contains("model"); | |
| let is_invalid = lower.contains("not found") | |
| || lower.contains("not a valid model") | |
| || (lower.contains("invalid model") && !lower.contains("invalid model parameter") && !lower.contains("invalid model option")) | |
| || (lower.contains("unknown model") && !lower.contains("unknown model parameter")) | |
| || lower.contains("is not a valid model name or alias"); | |
| if !(mentions_model && is_invalid) { | |
| return None; | |
| } | |
| Some(parse_invalid_model_name(response_text).unwrap_or_else(|| "unknown".to_string())) | |
| } |
References
- When classifying errors by matching substrings, avoid overly generic patterns like 'request failed' that can cause false positives. Prefer more specific markers like HTTP status codes or explicit error messages.
| fn parse_invalid_model_name(text: &str) -> Option<String> { | ||
| let idx = text.to_ascii_lowercase().find("model")?; | ||
| let after = &text[idx + "model".len()..]; | ||
| let mut chars = after.char_indices().skip_while(|(_, c)| c.is_whitespace()); | ||
| let (start, quote) = chars.next()?; | ||
| if quote != '\'' && quote != '"' { | ||
| return None; | ||
| } | ||
| let rest = &after[start + quote.len_utf8()..]; | ||
| let end = rest.find(quote)?; | ||
| let name = rest[..end].trim(); | ||
| (!name.is_empty()).then(|| name.to_string()) | ||
| } |
There was a problem hiding this comment.
The current implementation of parse_invalid_model_name fails to extract the model name if it is double-quoted in the JSON message (which is escaped as \" in the raw response_text), because the backslash \ becomes the first non-whitespace character after "model". Additionally, it fails if there is a colon separator (e.g., "Model: 'auto'"). We can make this extraction much more robust by skipping colons and backslashes during the search for the quote character, and then trimming any trailing backslashes from the extracted name.
| fn parse_invalid_model_name(text: &str) -> Option<String> { | |
| let idx = text.to_ascii_lowercase().find("model")?; | |
| let after = &text[idx + "model".len()..]; | |
| let mut chars = after.char_indices().skip_while(|(_, c)| c.is_whitespace()); | |
| let (start, quote) = chars.next()?; | |
| if quote != '\'' && quote != '"' { | |
| return None; | |
| } | |
| let rest = &after[start + quote.len_utf8()..]; | |
| let end = rest.find(quote)?; | |
| let name = rest[..end].trim(); | |
| (!name.is_empty()).then(|| name.to_string()) | |
| } | |
| fn parse_invalid_model_name(text: &str) -> Option<String> { | |
| let idx = text.to_ascii_lowercase().find("model")?; | |
| let after = &text[idx + "model".len()..]; | |
| let mut chars = after.char_indices().skip_while(|(_, c)| c.is_whitespace() || *c == ':' || *c == '\\'); | |
| let (start, quote) = chars.next()?; | |
| if quote != '\'' && quote != '"' { | |
| return None; | |
| } | |
| let rest = &after[start + quote.len_utf8()..]; | |
| let end = rest.find(quote)?; | |
| let name = rest[..end].trim_end_matches('\\').trim(); | |
| (!name.is_empty()).then(|| name.to_string()) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/ironclaw_llm/src/error.rs`:
- Around line 129-138: The model-name parser starting at line 129 searches for
"model" in the entire raw JSON text without proper context, causing it to
extract incorrect characters when param:"model" appears before the actual error
message. Fix this by first extracting the error.message field from the JSON,
then perform the model name extraction (finding the "model" keyword, skipping
whitespace, and extracting the quoted string) only within that message content
instead of the entire text. This ensures the parser anchors to the actual error
message rather than field definitions.
In `@crates/ironclaw_llm/src/nearai_chat.rs`:
- Around line 412-422: Add a regression test for the new ModelNotAvailable error
mapping behavior. Create a new tokio test function in the nearai_chat test
module that mocks an HTTP 400 response from the `/v1/chat/completions` endpoint
indicating an invalid model, then calls the complete() function and asserts that
the returned error is LlmError::ModelNotAvailable with the correct provider and
model fields, rather than LlmError::RequestFailed. This test should drive the
complete() function through the new code path in the diff that calls
crate::error::invalid_model_error() and verifies the mapping works correctly at
the caller level.
🪄 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: 19044d38-9138-4d2b-87da-c1a909fffd42
📒 Files selected for processing (2)
crates/ironclaw_llm/src/error.rscrates/ironclaw_llm/src/nearai_chat.rs
| let idx = text.to_ascii_lowercase().find("model")?; | ||
| let after = &text[idx + "model".len()..]; | ||
| let mut chars = after.char_indices().skip_while(|(_, c)| c.is_whitespace()); | ||
| let (start, quote) = chars.next()?; | ||
| if quote != '\'' && quote != '"' { | ||
| return None; | ||
| } | ||
| let rest = &after[start + quote.len_utf8()..]; | ||
| let end = rest.find(quote)?; | ||
| let name = rest[..end].trim(); |
There was a problem hiding this comment.
Model-name parser can return junk when param:"model" appears before the message
Line 129 anchors on the first "model" anywhere in the raw JSON, so field-order changes can extract "," (or similar) instead of the actual model id. Anchor extraction to error.message first, then parse quoted model text from that string.
Proposed fix
fn parse_invalid_model_name(text: &str) -> Option<String> {
- let idx = text.to_ascii_lowercase().find("model")?;
- let after = &text[idx + "model".len()..];
+ let message = serde_json::from_str::<serde_json::Value>(text)
+ .ok()
+ .and_then(|v| {
+ v.get("error")
+ .and_then(|e| e.get("message"))
+ .and_then(|m| m.as_str())
+ })
+ .unwrap_or(text);
+ let idx = message.to_ascii_lowercase().find("model")?;
+ let after = &message[idx + "model".len()..];
let mut chars = after.char_indices().skip_while(|(_, c)| c.is_whitespace());
let (start, quote) = chars.next()?;
if quote != '\'' && quote != '"' {
return None;
}🤖 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 `@crates/ironclaw_llm/src/error.rs` around lines 129 - 138, The model-name
parser starting at line 129 searches for "model" in the entire raw JSON text
without proper context, causing it to extract incorrect characters when
param:"model" appears before the actual error message. Fix this by first
extracting the error.message field from the JSON, then perform the model name
extraction (finding the "model" keyword, skipping whitespace, and extracting the
quoted string) only within that message content instead of the entire text. This
ensures the parser anchors to the actual error message rather than field
definitions.
| // HTTP 400 "model not found / not a valid model" — a permanent client | ||
| // error (e.g. NEARAI_MODEL=auto against a gateway with no `auto` | ||
| // alias). Map to the non-retryable ModelNotAvailable so the run fails | ||
| // fast with a clear message instead of retry-looping into a | ||
| // multi-minute silent hang (the "answer never appears" report). | ||
| if let Some(model) = crate::error::invalid_model_error(status_code, &response_text) { | ||
| return Err(LlmError::ModelNotAvailable { | ||
| provider: "nearai_chat".to_string(), | ||
| model, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add caller-level regression test for the new ModelNotAvailable mapping
This behavior change is in the request path, but there’s no nearai_chat regression test asserting that a 400 invalid-model response from /v1/chat/completions becomes LlmError::ModelNotAvailable (and not RequestFailed). Add a #[tokio::test] that drives complete() through this branch.
As per coding guidelines, “Every bug fix must include a regression test” and invariant “Test through the caller … not only the helper.”
🤖 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 `@crates/ironclaw_llm/src/nearai_chat.rs` around lines 412 - 422, Add a
regression test for the new ModelNotAvailable error mapping behavior. Create a
new tokio test function in the nearai_chat test module that mocks an HTTP 400
response from the `/v1/chat/completions` endpoint indicating an invalid model,
then calls the complete() function and asserts that the returned error is
LlmError::ModelNotAvailable with the correct provider and model fields, rather
than LlmError::RequestFailed. This test should drive the complete() function
through the new code path in the diff that calls
crate::error::invalid_model_error() and verifies the mapping works correctly at
the caller level.
Source: Coding guidelines
…g (F8) A 400 'model not found / not a valid model' (e.g. NEARAI_MODEL=auto sent to a cloud-api.near.ai gateway that has no `auto` alias) fell through to the retryable RequestFailed path, so it was retried 3x at the provider layer and again at the loop layer — turning a one-line model-config mistake into a multi-minute silent hang where the user's turn never produced a reply (the 'answers don't show up' report). Live repro: with model=auto every turn hung 120s+ with no reply; with a real model id (anthropic/claude-haiku-4-5 via cloud-api.near.ai) the same turns answered in ~4s. Add error::invalid_model_error (mirrors context_length_error) and map a 400 invalid-model in nearai_chat to the non-retryable LlmError::ModelNotAvailable, so the run fails fast with a clear 'model not available' message (which the WebChat v2 timeline already surfaces) instead of hanging. Guards against misreading a context-length 400. Does NOT change what `auto` resolves to — that's the product decision tracked separately. cargo test -p ironclaw_llm: 875 passed, 0 failed (2 new). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
40e0494 to
91aec53
Compare
ilblackdragon
left a comment
ilblackdragon
left a comment
There was a problem hiding this comment.
Review — VERDICT: APPROVE WITH NITS (batch review pass)
Well-targeted fix: maps an "invalid/unknown model" HTTP 400 from NEAR AI to the non-retryable ModelNotAvailable, breaking the multi-minute retry/failover hang.
Relevance: still relevant — invalid_model_error/resolve_nearai_model are absent from current main (129ce21), not merged or superseded. Mergeable: yes, CI green.
Verified correct: today all non-context/non-5xx 400s → RequestFailed, which is_retryable() treats as retryable (retry.rs:45-59) → both retry and failover loop on a permanent client error = the reported hang. ModelNotAvailable is non-retryable (retry.rs:40, failover.rs:1132), so the remap fails fast. Ordering is right — context_length_error runs before the new branch, so a context-length 400 can't be misread as invalid-model. Tests are pure string tests (deterministic, no network); no unwrap/panic on the parse path.
Findings:
- Low —
parse_invalid_model_namelatches onto the first "model" token; with"param":"model"ordered before the message it can extract a stray comma as the model name (display-only cosmetic; the real near.ai body works). Anchor extraction to the invalidity phrase. - Low —
contains("model")+"not found"is body-substring-brittle; a non-model 400 mentioning both gets reclassified (harmless since 400s are non-retryable either way, but it swaps the informative body for a generic message). - Info —
"is not a valid model name or alias"is a dead superset of"not a valid model". - Scope — the shared helper is only wired into
nearai_chat.rs; codex/copilot/bedrock still map invalid-model 400 → retryableRequestFailed.
Land together with #5045 (same author): #5045 prevents the bad request (auto→concrete model), this PR is the fail-fast net for any future bad model. No file/text conflict.
🤖 batch PR-review pass via parallel-pr-review
3 participants
A 400 'model not found / not a valid model' (e.g. NEARAI_MODEL=auto sent to a
cloud-api.near.ai gateway that has no
autoalias) fell through to theretryable RequestFailed path, so it was retried 3x at the provider layer and
again at the loop layer — turning a one-line model-config mistake into a
multi-minute silent hang where the user's turn never produced a reply (the
'answers don't show up' report). Live repro: with model=auto every turn hung
120s+ with no reply; with a real model id (anthropic/claude-haiku-4-5 via
cloud-api.near.ai) the same turns answered in ~4s.
Add error::invalid_model_error (mirrors context_length_error) and map a 400
invalid-model in nearai_chat to the non-retryable LlmError::ModelNotAvailable,
so the run fails fast with a clear 'model not available' message (which the
WebChat v2 timeline already surfaces) instead of hanging. Guards against
misreading a context-length 400. Does NOT change what
autoresolves to —that's the product decision tracked separately.
cargo test -p ironclaw_llm: 875 passed, 0 failed (2 new).
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com