fix(ai): tool error handling#2671
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRenamed local variables in the chat tool-loop, changed how tool call responses are turned into text, added tracing error logs for tool failures, simplified JSON-vs-error classification for tool responses in OpenAI message conversion, and updated a test fixture and debug output. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
ehayes2000
force-pushed
the
fix-tool-errors
branch
from
88a7cc3 to
52a95e6
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/ai/src/types/providers/openai/message.rs`:
- Around line 381-392: The serializer for AssistantMessagePart::ToolCallErr is
emitting a JSON object (e.g., {"type":"error","description":...}) while the
deserializer (in the match building assistant_part) treats any parseable JSON as
ToolCallResponseJson, breaking roundtrips; update the outgoing serialization
code that constructs the tool message (the branch that emits ToolCallErr,
referenced by AssistantMessagePart::ToolCallErr and the tool_response_text
logic) to write plain description text (the same plain-text fail.description /
"Error calling tool: ..." string used in agent.rs) instead of wrapping it in a
JSON object so the incoming serde_json::from_str will fail and land in the Err
arm; also remove the now-unused ToolResponseParseError struct if present.
🪄 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
Run ID: 3a1bf1bd-57ab-4773-b20b-f2b8f5bdc66a
📒 Files selected for processing (2)
rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rsrust/cloud-storage/ai/src/types/providers/openai/message.rs
ehayes2000
force-pushed
the
fix-tool-errors
branch
2 times, most recently
from
05be68a to
0de6c43
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rust/cloud-storage/ai/src/types/providers/openai/message.rs (1)
367-377:⚠️ Potential issue | 🟠 MajorAvoid using JSON parseability as the only error/success discriminator.
An error description that is itself valid JSON will roundtrip as
ToolCallResponseJsoninstead ofToolCallErrbecauseserde_json::from_strsucceeds. For example, a tool failure description like{"error":"permission denied"}ornullloses its error semantics here. Consider preserving an explicit error marker/type channel or adding a non-JSON error envelope/prefix before relying on this parser. This is related to the prior roundtrip concern but remains possible for valid-JSON descriptions.Suggested regression test
#[test] fn test_assistant_with_tool_error_roundtrip() { @@ } + +#[test] +fn test_assistant_with_json_like_tool_error_roundtrip() { + let tool_call_id = "call_123".to_string(); + let tool_name = "get_weather".to_string(); + let original = ChatMessage { + role: Role::Assistant, + content: ChatMessageContent::AssistantMessageParts(vec![ + AssistantMessagePart::ToolCallErr { + name: tool_name.clone(), + description: r#"{"error":"permission denied"}"#.to_string(), + id: tool_call_id.clone(), + }, + ]), + image_urls: None, + }; + + let openai_msgs: Vec<ChatCompletionRequestMessage> = original.clone().into(); + let mut tool_mapping = HashMap::new(); + tool_mapping.insert(tool_call_id, tool_name); + + let converted_back = + convert_message(openai_msgs.into_iter().next().unwrap(), Some(&tool_mapping)); + + assert_eq!(original, converted_back); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/ai/src/types/providers/openai/message.rs` around lines 367 - 377, The code currently treats any valid JSON response_text as success by using serde_json::from_str in the AssistantMessagePart creation, which misclassifies JSON-formatted error descriptions; change the decision logic in the block that builds AssistantMessagePart (around response_text, tool_msg, and AssistantMessagePart::ToolCallResponseJson/ToolCallErr) to first consult an explicit error indicator from the tool call (e.g., a flag on tool_msg like is_error/ok or a dedicated error field) or require a non-JSON error envelope/prefix (e.g., "ERROR:"/{"__error":...}) before parsing JSON; only call serde_json::from_str when the explicit indicator says success (or after stripping/validating a non-JSON envelope), otherwise construct ToolCallErr with the raw response_text and id so genuine error semantics are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/ai/src/types/providers/openai/message.rs`:
- Around line 589-590: The test contains an unconditional debug print
eprintln!("{:#?}", openai_msgs); which pollutes test output; remove that line
(or replace it with a conditional/log macro if you truly need debug only under a
debug flag) so the test uses only the existing assertion; locate the creation
flow where openai_msgs is set (the let openai_msgs:
Vec<ChatCompletionRequestMessage> = original.clone().into(); line in message.rs)
and delete the eprintln! call.
---
Duplicate comments:
In `@rust/cloud-storage/ai/src/types/providers/openai/message.rs`:
- Around line 367-377: The code currently treats any valid JSON response_text as
success by using serde_json::from_str in the AssistantMessagePart creation,
which misclassifies JSON-formatted error descriptions; change the decision logic
in the block that builds AssistantMessagePart (around response_text, tool_msg,
and AssistantMessagePart::ToolCallResponseJson/ToolCallErr) to first consult an
explicit error indicator from the tool call (e.g., a flag on tool_msg like
is_error/ok or a dedicated error field) or require a non-JSON error
envelope/prefix (e.g., "ERROR:"/{"__error":...}) before parsing JSON; only call
serde_json::from_str when the explicit indicator says success (or after
stripping/validating a non-JSON envelope), otherwise construct ToolCallErr with
the raw response_text and id so genuine error semantics are preserved.
🪄 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
Run ID: 3d689c84-f331-44be-bb81-bf6d975a38ce
📒 Files selected for processing (3)
rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/test.rsrust/cloud-storage/ai/src/types/providers/openai/message.rs
| let openai_msgs: Vec<ChatCompletionRequestMessage> = original.clone().into(); | ||
| eprintln!("{:#?}", openai_msgs); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the unconditional debug print from the test.
eprintln! will add noise under --nocapture or when the test fails; the assertion already prints the diff context.
Clean up debug output
let openai_msgs: Vec<ChatCompletionRequestMessage> = original.clone().into();
- eprintln!("{:#?}", openai_msgs);
// Create tool call mapping for conversion back📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let openai_msgs: Vec<ChatCompletionRequestMessage> = original.clone().into(); | |
| eprintln!("{:#?}", openai_msgs); | |
| let openai_msgs: Vec<ChatCompletionRequestMessage> = original.clone().into(); | |
| // Create tool call mapping for conversion back |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/types/providers/openai/message.rs` around lines 589
- 590, The test contains an unconditional debug print eprintln!("{:#?}",
openai_msgs); which pollutes test output; remove that line (or replace it with a
conditional/log macro if you truly need debug only under a debug flag) so the
test uses only the existing assertion; locate the creation flow where
openai_msgs is set (the let openai_msgs: Vec<ChatCompletionRequestMessage> =
original.clone().into(); line in message.rs) and delete the eprintln! call.
ehayes2000
force-pushed
the
fix-tool-errors
branch
from
0de6c43 to
add86ac
Compare
1 participant
No description provided.