Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a boolean Message.is_error and threads it from tool execution through streaming, LLM adapters, session replay (acp), docs, and tests so tool results preserve completed/failed status. ChangesTool-result error propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
![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 `@src/acp/mod.rs`:
- Around line 250-259: The code silently swallows JSON parse errors for tool
call arguments by using serde_json::from_str(...).ok() — change this to
explicitly attempt parsing (e.g., match or map_err) on tc.function.arguments so
parsing failures are logged (include tc.id and tc.function.name for context)
before falling back to None; update the raw_input creation used in
on_update(SessionUpdate::ToolCall(...)) to use the parsed value on success and
still pass None on failure after logging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3268db63-6a06-415d-b616-af8c64c7c316
📒 Files selected for processing (1)
src/acp/mod.rs
Add is_error: bool to Message so tool failures are stored explicitly in conversation history rather than being lost as plain text. Propagate the flag through StreamEvent::ToolResult and agent.rs so both the live path and history replay emit ToolCallStatus::Failed instead of always Completed. Forward is_error to Anthropic's tool_result block so the LLM gets accurate signal on failure.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
src/core/llm/gemini.rs (1)
158-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGemini adapter must propagate
msg.is_errorintoFunctionResponse.response(not always{"result": ...})
src/core/llm/gemini.rs’sRole::Toolbranch ignoresmsg.is_errorand always shapesGeminiFunctionResponse.responseas{"result": msg.content...}. TheMessagemodel explicitly storesis_errorfor replay correctness, and the Anthropic adapter forwards it viatool_result.is_error. Gemini should encode the failure in the free-formfunction_response.responsepayload (e.g.,{"error": ...}) so the model can distinguish errors from successes.♻️ Suggested payload shape on error
- let part = GeminiPart { - text: None, - function_call: None, - function_response: Some(GeminiFunctionResponse { - name, - response: serde_json::json!({ - "result": msg.content.clone().unwrap_or_default() - }), - }), - }; + let content = msg.content.clone().unwrap_or_default(); + let response = if msg.is_error { + serde_json::json!({ "error": content }) + } else { + serde_json::json!({ "result": content }) + }; + let part = GeminiPart { + text: None, + function_call: None, + function_response: Some(GeminiFunctionResponse { name, response }), + };🤖 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 `@src/core/llm/gemini.rs` around lines 158 - 188, The Role::Tool branch currently always sets GeminiFunctionResponse.response to {"result": ...} and ignores Message::is_error; update the logic that builds GeminiFunctionResponse in src/core/llm/gemini.rs (the Role::Tool case where GeminiPart and GeminiFunctionResponse are constructed) to check msg.is_error and, when true, set response to an error-shaped payload (e.g., {"error": msg.content.unwrap_or_default()}) and otherwise keep {"result": msg.content.unwrap_or_default()}; preserve existing merging into last GeminiContent and ensure None content becomes an empty string.
🧹 Nitpick comments (3)
src/core/agent.rs (1)
91-95: ⚡ Quick win
ToolExecutionResultdrops theis_errorsignal in the non-streaming result path.
is_erroris now threaded throughStreamEvent::ToolResultandMessage::tool_result, but theToolExecutionResultpushed intoAgentStep.tool_callshere is unchanged. Callers ofrun_agent_with_historythat consumeAgentResult.steps(i.e., not using the streaming callback) still need to inspect the"Error: "prefix onresultto know whether the tool failed — the exact heuristic the cohort is trying to retire.For consistency, add
is_error: booltoToolExecutionResultinsrc/core/models.rsand populate it here.♻️ Proposed change
In
src/core/models.rs:#[derive(Debug, Serialize, Clone)] pub struct ToolExecutionResult { pub tool_name: String, pub arguments: String, pub result: String, + pub is_error: bool, }In
src/core/agent.rs:tool_results.push(ToolExecutionResult { tool_name: tool_name.clone(), arguments: arguments.clone(), result: result.clone(), + is_error, });🤖 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 `@src/core/agent.rs` around lines 91 - 95, Add an is_error: bool field to the ToolExecutionResult struct in models (so it mirrors StreamEvent::ToolResult / Message::tool_result) and populate that field when pushing into tool_results inside the agent logic: update the ToolExecutionResult construction in the code that currently builds tool_results (the push that uses tool_name, arguments, result) to include is_error (use the existing is_error variable in scope or derive it the same way streaming path does). Ensure the struct definition and all places that construct ToolExecutionResult are updated to compile.src/core/models.rs (1)
381-391: ⚡ Quick winConsider adding tests for
is_error: trueand serde default round-trip.The only assertion added covers the
falsepath. Given the PR explicitly relies on#[serde(default)]so older persisted history withoutis_errordeserializes cleanly, and onis_error: truebeing faithfully replayed, a couple of small tests would lock that contract in:
Message::tool_result(..., true)setsis_error == trueand serializes to JSON that contains"is_error": true.- Deserializing a
MessageJSON without theis_errorkey yieldsis_error == false.Messagewithis_error: falsedoes not emit the key (matchesskip_serializing_if).🧪 Suggested additional tests
#[test] fn message_tool_result_sets_correct_fields() { let msg = Message::tool_result("call_1".into(), "read_file".into(), "content".into(), false); assert_eq!(msg.role, Role::Tool); assert_eq!(msg.content.as_deref(), Some("content")); assert_eq!(msg.tool_call_id.as_deref(), Some("call_1")); assert_eq!(msg.tool_name.as_deref(), Some("read_file")); assert!(!msg.is_error); assert!(msg.tool_calls.is_none()); } + + #[test] + fn message_tool_result_propagates_is_error_true() { + let msg = + Message::tool_result("call_2".into(), "shell".into(), "boom".into(), true); + assert!(msg.is_error); + let json: Value = serde_json::to_value(&msg).unwrap(); + assert_eq!(json["is_error"], true); + } + + #[test] + fn message_is_error_skipped_when_false_and_defaults_on_deserialize() { + let msg = Message::tool_result("c".into(), "t".into(), "ok".into(), false); + let json: Value = serde_json::to_value(&msg).unwrap(); + assert!(json.get("is_error").is_none()); + + let raw = r#"{"role":"tool","content":"ok","tool_call_id":"c","tool_name":"t"}"#; + let parsed: Message = serde_json::from_str(raw).unwrap(); + assert!(!parsed.is_error); + }🤖 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 `@src/core/models.rs` around lines 381 - 391, Add tests to cover the true-path and serde default round-trip for is_error: call Message::tool_result(..., true) and assert msg.is_error == true and that serializing the Message to JSON contains the key "is_error": true; also add a test that deserializing a JSON Message without an "is_error" field yields is_error == false (verifying #[serde(default)]), and a test that a Message with is_error == false does not emit the "is_error" key when serialized (verifying skip_serializing_if). Use the existing message_tool_result_sets_correct_fields test as a template to create tests referencing Message::tool_result, is_error, and serde (serialize/deserialize) helpers.src/acp/mod.rs (1)
259-276: 💤 Low valueConsider using
InProgressfor the replayed tool_call to match the live streaming lifecycle.In the live path (
acp_prompt, line 147), a new tool call is first emitted withToolCallStatus::InProgressand then transitioned viaToolCallUpdate. Here in replay, the initialToolCallis emitted asCompleted(line 272), and then immediately followed by aToolCallUpdatefrom theRole::Toolbranch that may flip it toFailed. ACompleted → Failedtransition for the same id is awkward for clients tracking status state machines (see the lifecycle documented indocs/api.md:pending → in_progress → completed/failed).Emitting
InProgresshere and letting the subsequentRole::Toolupdate decide the terminal state keeps replay consistent with the live flow.♻️ Proposed change
on_update(SessionUpdate::ToolCall( AcpToolCall::new(tc.id.clone(), &tc.function.name) - .status(ToolCallStatus::Completed) + .status(ToolCallStatus::InProgress) .raw_input(raw_input), ));🤖 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 `@src/acp/mod.rs` around lines 259 - 276, The replay currently emits a ToolCall with ToolCallStatus::Completed which can produce an invalid Completed→Failed transition; change the initial emitted status to ToolCallStatus::InProgress when constructing the AcpToolCall in the on_update(SessionUpdate::ToolCall(...)) call (the code that calls AcpToolCall::new(tc.id.clone(), &tc.function.name).status(...).raw_input(...)); leave the Role::Tool branch updates as-is so they can transition the same id to Failed or Completed, ensuring the replay matches the live lifecycle emitted by acp_prompt.
🤖 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 `@docs/api.md`:
- Line 755: The documentation and implementation disagree: replayed tool calls
are emitted by acp_load_session with ToolCallStatus::Completed (currently at the
replay emission in acp_load_session), but the docs state they arrive as
tool_call with status "in_progress"; to fix, change the replay emission in
acp_load_session to emit ToolCallStatus::InProgress for assistant tool-call
requests (instead of ToolCallStatus::Completed) so the initial replay matches
the "in_progress" doc behavior, and update any tests or downstream handling that
expect Completed; alternatively, if you prefer to keep the code behavior, update
the docs sentence to state replayed tool calls are emitted as "completed" and
may later be flipped by tool_call_update when is_error is set.
---
Outside diff comments:
In `@src/core/llm/gemini.rs`:
- Around line 158-188: The Role::Tool branch currently always sets
GeminiFunctionResponse.response to {"result": ...} and ignores
Message::is_error; update the logic that builds GeminiFunctionResponse in
src/core/llm/gemini.rs (the Role::Tool case where GeminiPart and
GeminiFunctionResponse are constructed) to check msg.is_error and, when true,
set response to an error-shaped payload (e.g., {"error":
msg.content.unwrap_or_default()}) and otherwise keep {"result":
msg.content.unwrap_or_default()}; preserve existing merging into last
GeminiContent and ensure None content becomes an empty string.
---
Nitpick comments:
In `@src/acp/mod.rs`:
- Around line 259-276: The replay currently emits a ToolCall with
ToolCallStatus::Completed which can produce an invalid Completed→Failed
transition; change the initial emitted status to ToolCallStatus::InProgress when
constructing the AcpToolCall in the on_update(SessionUpdate::ToolCall(...)) call
(the code that calls AcpToolCall::new(tc.id.clone(),
&tc.function.name).status(...).raw_input(...)); leave the Role::Tool branch
updates as-is so they can transition the same id to Failed or Completed,
ensuring the replay matches the live lifecycle emitted by acp_prompt.
In `@src/core/agent.rs`:
- Around line 91-95: Add an is_error: bool field to the ToolExecutionResult
struct in models (so it mirrors StreamEvent::ToolResult / Message::tool_result)
and populate that field when pushing into tool_results inside the agent logic:
update the ToolExecutionResult construction in the code that currently builds
tool_results (the push that uses tool_name, arguments, result) to include
is_error (use the existing is_error variable in scope or derive it the same way
streaming path does). Ensure the struct definition and all places that construct
ToolExecutionResult are updated to compile.
In `@src/core/models.rs`:
- Around line 381-391: Add tests to cover the true-path and serde default
round-trip for is_error: call Message::tool_result(..., true) and assert
msg.is_error == true and that serializing the Message to JSON contains the key
"is_error": true; also add a test that deserializing a JSON Message without an
"is_error" field yields is_error == false (verifying #[serde(default)]), and a
test that a Message with is_error == false does not emit the "is_error" key when
serialized (verifying skip_serializing_if). Use the existing
message_tool_result_sets_correct_fields test as a template to create tests
referencing Message::tool_result, is_error, and serde (serialize/deserialize)
helpers.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1918326d-3e22-4fa2-a438-7b4375605fd5
📒 Files selected for processing (8)
docs/api.mdsrc/acp/mod.rssrc/core/agent.rssrc/core/llm/anthropic.rssrc/core/llm/gemini.rssrc/core/llm/retry.rssrc/core/models.rssrc/rag/prompt.rs
1 participant
Summary by CodeRabbit