Repository navigation
Handle malformed NEAR streaming tool args as repairable - #5867
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummary by CodeRabbit
WalkthroughStreaming tool-call argument parsing now preserves malformed arguments as parse errors. The gateway rejects those tool calls as invalid output, marks them repairable, and reconstructs repair requests with an invalid-arguments payload. Tests cover batch and streaming repair paths. ChangesMalformed tool-call arguments handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
scope: docs
84dc29b to
d57a255
Compare
![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 streaming support across the LLM provider, gateway, and WebUI layers, specifically implementing OpenAI-compatible SSE streaming for the NEAR AI chat provider. It adds streaming retry logic, progress-tracking sinks, and event subscription capabilities to allow live assistant text updates to stream directly to the browser. Feedback on the changes highlights a performance and network efficiency bottleneck in ProviderStreamSink::text_delta, where cloning, sanitizing, and sending the entire accumulated text on every delta results in quadratic complexity.
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.
I am having trouble creating individual review comments. Click here to see my feedback.
crates/ironclaw_reborn/src/model_gateway.rs (1065-1080)
Performance & Network Efficiency Bottleneck ($O(N^2)$ Complexity)
In ProviderStreamSink::text_delta, on every single token/character delta received from the LLM provider, the entire accumulated string is cloned and passed to sanitize_model_visible_text (which performs regex/pattern scanning for secrets), and then the entire accumulated string is sent over the network via safe_text_update.
This introduces two major issues:
-
CPU Overhead: For a response of
$N$ tokens, cloning and scanning the entire growing string on every delta results in$O(N^2)$ complexity, which will severely bottleneck CPU performance on long responses. - Network Bandwidth: Since the full accumulated text is sent on every single token, the total data transferred over the SSE connection grows quadratically.
Recommendation
If this implementation is intentionally inefficient for the sake of initial correctness, please document the deferred optimization (e.g., incremental updates) as a known future task. Otherwise, consider implementing a throttling mechanism or incremental updates to avoid the
References
- When an implementation is intentionally inefficient for the sake of initial correctness, document the deferred optimization (e.g., incremental updates) as a known future task.
github-actions
Bot
added
size: M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 1826-1834: Use a fixed safe summary for provider parse errors in
provider_tool_arguments_parse_error_summary and
HostManagedModelError::safe_summary: do not return any provider-authored parse
text even when it matches the failed to parse tool-call arguments JSON prefix,
and always fall back to PROVIDER_TOOL_ARGUMENTS_INVALID_SUMMARY. Keep the
detailed parse_error content only in the repair-message path that builds the
tool-call retry message, so the summary stays fully fixed and safe while the
repair flow still gets the full error context.
🪄 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: 80c86249-f3c6-4357-a12e-0c38d54347a1
📒 Files selected for processing (3)
crates/ironclaw_llm/src/nearai_chat.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/tests/llm_gateway.rs
d57a255 to
d06d2ab
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/ironclaw_reborn/src/model_gateway.rs (1)
1827-1837: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSafe-summary leak: provider parse-error text still passes through unchanged.
This reintroduces the exact issue flagged on a prior commit of this PR: when the sanitized parse error starts with the prefix and passes
LoopSafeSummary::new, it's returned verbatim as thesafe_summary.sanitize_model_visible_textonly redacts secret-shaped tokens, not arbitrary provider-authored text, so ordinary serde_json messages (line/column info, "trailing characters", etc.) leak through as host safe-summary content. As per path instructions, "When constructingCapabilityResultMessage.safe_summaryor any host safe-summary, use fixed host-authored text only; do not interpolate model/user-controlled data such as ... provider strings." The detailed message is already carried separately through the repair-message path, so this passthrough isn't needed.🔒 Proposed fix: always return the fixed summary
fn provider_tool_arguments_parse_error_summary(parse_error: &str) -> String { - let sanitized = sanitize_model_visible_text(parse_error.to_string()); - let sanitized = sanitized.trim(); - if sanitized.starts_with(PROVIDER_TOOL_ARGUMENTS_PARSE_ERROR_PREFIX) - && LoopSafeSummary::new(sanitized).is_ok() - { - sanitized.to_string() - } else { - PROVIDER_TOOL_ARGUMENTS_INVALID_SUMMARY.to_string() - } + let _ = parse_error; // detailed text flows through the repair-message path only + PROVIDER_TOOL_ARGUMENTS_INVALID_SUMMARY.to_string() }🤖 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_reborn/src/model_gateway.rs` around lines 1827 - 1837, The safe-summary helper currently lets provider-authored parse-error text pass through unchanged when it matches the prefix and satisfies LoopSafeSummary::new. Update provider_tool_arguments_parse_error_summary to always return the fixed host-authored PROVIDER_TOOL_ARGUMENTS_INVALID_SUMMARY instead of forwarding sanitized parse_error content, keeping any detailed provider text only in the separate repair-message path.Source: Path instructions
🤖 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.
Duplicate comments:
In `@crates/ironclaw_reborn/src/model_gateway.rs`:
- Around line 1827-1837: The safe-summary helper currently lets
provider-authored parse-error text pass through unchanged when it matches the
prefix and satisfies LoopSafeSummary::new. Update
provider_tool_arguments_parse_error_summary to always return the fixed
host-authored PROVIDER_TOOL_ARGUMENTS_INVALID_SUMMARY instead of forwarding
sanitized parse_error content, keeping any detailed provider text only in the
separate repair-message path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5bc59c3-e84a-44bb-95c2-a090fade6e64
📒 Files selected for processing (3)
crates/ironclaw_llm/src/nearai_chat.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/tests/llm_gateway.rs
|
🚅 Deployed to the ironclaw-pr-5867 environment in ironclaw-ci-preview
|
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.13% — 283983 / 333600 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
d06d2ab to
122004c
Compare
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)
crates/ironclaw_llm/src/nearai_chat.rs (1)
1562-1593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
into_tool_callis now infallible but still returnsResult.
parse_tool_call_args_lossy(crates/ironclaw_llm/src/tool_args.rs:60-71) never returns anErr, so every path ininto_tool_callnow producesOk(...). TheResult<Option<ToolCall>, LlmError>signature and the caller'sstate.into_tool_call()?(line 641) are vestigial and imply a failure mode that no longer exists.♻️ Proposed simplification
- fn into_tool_call(self) -> Result<Option<ToolCall>, LlmError> { + fn into_tool_call(self) -> Option<ToolCall> { if self.id.is_empty() && self.name.is_empty() && self.arguments.is_empty() && !self.arguments_delta_seen { - return Ok(None); + return None; } let (arguments, arguments_parse_error) = if self.arguments.is_empty() && !self.arguments_delta_seen { (serde_json::Value::Object(Default::default()), None) } else { parse_tool_call_args_lossy(&self.arguments) }; - Ok(Some(ToolCall { + Some(ToolCall { id: self.id, name: self.name, arguments, reasoning: None, signature: None, arguments_parse_error, - })) + }) }And at the call site:
if let Some(tool_call) = state.into_tool_call() { ... }.🤖 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 1562 - 1593, `NearAiStreamingToolCallState::into_tool_call` is now infallible, so remove the unnecessary `Result`/`LlmError` from its signature and return `Option<ToolCall>` directly. Update the logic to keep returning `None` for the empty state and `Some(ToolCall { ... })` otherwise, and change the caller that currently uses `state.into_tool_call()?` to handle the plain `Option` (for example with `if let Some(tool_call) = ...`). Also remove any now-unused `LlmError` import or related error handling in `nearai_chat.rs`.
🤖 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_reborn/src/model_gateway.rs`:
- Around line 1941-1950: The repairability check in
is_provider_tool_arguments_parse_error_summary is stringly-typed and can
silently break if safe_summary wording changes. Update
HostManagedModelError/HostManagedModelErrorKind handling so
is_repairable_provider_tool_output_error and
is_provider_tool_arguments_parse_error_summary rely on a typed enum variant or
dedicated error kind instead of starts_with/== on safe_summary, then keep the
InvalidOutput classification wired through that typed signal.
---
Outside diff comments:
In `@crates/ironclaw_llm/src/nearai_chat.rs`:
- Around line 1562-1593: `NearAiStreamingToolCallState::into_tool_call` is now
infallible, so remove the unnecessary `Result`/`LlmError` from its signature and
return `Option<ToolCall>` directly. Update the logic to keep returning `None`
for the empty state and `Some(ToolCall { ... })` otherwise, and change the
caller that currently uses `state.into_tool_call()?` to handle the plain
`Option` (for example with `if let Some(tool_call) = ...`). Also remove any
now-unused `LlmError` import or related error handling in `nearai_chat.rs`.
🪄 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: 1bf6d69f-db21-43b6-8d7b-836bb4f344e9
📒 Files selected for processing (3)
crates/ironclaw_llm/src/nearai_chat.rscrates/ironclaw_reborn/src/model_gateway.rscrates/ironclaw_reborn/tests/llm_gateway.rs
| fn is_repairable_provider_tool_output_error(error: &HostManagedModelError) -> bool { | ||
| error.kind == HostManagedModelErrorKind::InvalidOutput | ||
| && is_provider_arguments_too_large_summary(&error.safe_summary) | ||
| && (is_provider_arguments_too_large_summary(&error.safe_summary) | ||
| || is_provider_tool_arguments_parse_error_summary(&error.safe_summary)) | ||
| } | ||
|
|
||
| fn is_provider_tool_arguments_parse_error_summary(safe_summary: &str) -> bool { | ||
| safe_summary.starts_with(PROVIDER_TOOL_ARGUMENTS_PARSE_ERROR_PREFIX) | ||
| || safe_summary == PROVIDER_TOOL_ARGUMENTS_INVALID_SUMMARY | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoff
Repairability classification keyed off safe_summary string equality.
is_provider_tool_arguments_parse_error_summary matches on starts_with/== against the safe-summary string rather than a typed error variant/kind on HostManagedModelError. This mirrors the pre-existing is_provider_arguments_too_large_summary pattern, so it's consistent with current file conventions, but as per coding guidelines, "If code matches on string literals, the underlying type should be an enum; do not leave it as a stringly-typed value." Any future safe-summary wording change silently breaks the retry classification with no compiler signal.
🤖 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_reborn/src/model_gateway.rs` around lines 1941 - 1950, The
repairability check in is_provider_tool_arguments_parse_error_summary is
stringly-typed and can silently break if safe_summary wording changes. Update
HostManagedModelError/HostManagedModelErrorKind handling so
is_repairable_provider_tool_output_error and
is_provider_tool_arguments_parse_error_summary rely on a typed enum variant or
dedicated error kind instead of starts_with/== on safe_summary, then keep the
InvalidOutput classification wired through that typed signal.
Source: Coding guidelines
122004c to
f87811a
Compare
github-actions
Bot
added
size: L
This branch was successfully deployed
1 participant
Summary
arguments_parse_errorinstead of failing the provider streamargumentsfields as malformed while preserving the existing omitted-arguments{}fallbackRoot cause
The streamed NEAR parser concatenated tool-call argument deltas and strict-parsed them after
[DONE]. If NEAR emitted malformed JSON after assistant text had already streamed, the provider returnedInvalidResponse; the retry layer refused to retry after emitted text, so the run failed instead of giving the model a repair turn. A present-but-empty streamedargumentsfield also collapsed to{}, which could bypass the repair metadata.Recovery behavior
For malformed streamed NEAR tool arguments, the gateway now rejects the whole provider tool-call batch before any capability is registered or executed. The repair request sent back to the model replays the assistant tool call with safe placeholder arguments, then attaches a tool result containing the parse failure plus the full raw malformed argument text verbatim. That raw detail is model-only repair context; safe summaries/logs use only the sanitized first parse-error line.
Testing tier note
The malformed-argument repair behavior sits at the provider/gateway seam before capability registration. The Reborn integration harness does not currently expose a production-wired fixture that can inject provider-internal
ToolCall.arguments_parse_errormetadata from a streamed provider response. The crate-tierllm_gatewaytest drives the productionLlmProviderModelGateway::stream_model_with_capabilities_and_progresscaller boundary with a streaming provider fake and the real gateway capability surface fake, which is the narrowest reachable seam for this provider metadata path without adding test-only integration wiring.Validation
cargo fmt --checkgit diff --checkcargo test -p ironclaw_llm complete_with_tools_streaming -- --nocapturecargo test -p ironclaw_reborn --features root-llm-provider,libsql-secrets,libsql-restart-tests,webui-user-store provider_tool_arguments_parse_error_summary -- --nocapturecargo test -p ironclaw_reborn --features root-llm-provider,libsql-secrets,libsql-restart-tests,webui-user-store --test llm_gateway gateway_repairs -- --nocapturecargo clippy -p ironclaw_llm --all-targets --all-features -- -D warningscargo clippy -p ironclaw_reborn --features root-llm-provider,libsql-secrets,libsql-restart-tests,webui-user-store --test llm_gateway -- -D warnings