Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoved the Changes
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/tool/tool_loop/chat/agent.rs`:
- Around line 405-406: Add the #[must_use] attribute to the pure function
correct_message_chain so callers are warned if they ignore its returned
corrected message list; place #[must_use] directly above the function
declaration (the existing #[tracing::instrument(skip_all)] can remain, so add
#[must_use] on its own line immediately above or below the tracing attribute to
ensure the compiler enforces use of the return value from correct_message_chain.
- Line 404: Update the inline comment that currently reads "anthropic sometimes
doesn't return responses for their servier tools. this causes 400's" to correct
the typo "servier" → "server" so it reads "anthropic sometimes doesn't return
responses for their server tools. this causes 400's"; locate and edit the
comment in the agent.rs chat agent code (the comment that begins with "anthropic
sometimes...") to apply this single-word fix.
- Around line 366-372: In the ToolCall::try_from error branch inside the agent
loop, change the tracing field name from err=?e to error=?e in the
tracing::error! invocation so it follows the project's logging convention (i.e.,
use tracing::error!(error=?e, "ToolCall::try_from failed from {:#?}", call)); do
not interpolate the error into the message string.
- Around line 111-117: The loop currently checks for Err with `if let Err(e)`
and then calls `item.unwrap()`, which is non-idiomatic; change the `while let
Some(item) = stream.next().await { ... }` body to use a `match item { Err(e) =>
{ yield Err(e); break 'outer; }, Ok(part_or_ext) => { /* use part_or_ext here */
} }` (or a `let Ok(part_or_ext) = item else { yield Err(e); break 'outer; }`
style) so you never call `.unwrap()` and the `part_or_ext` binding is produced
directly and safely in the `agent.rs` loop handling the stream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ba79930-5441-4089-924b-d102a8e0584c
📒 Files selected for processing (7)
rust/cloud-storage/ai/src/tool/tool_loop/ai_client.rsrust/cloud-storage/ai/src/tool/tool_loop/chained.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/mod.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/test.rsrust/cloud-storage/ai/src/tool/tool_loop/constant.rsrust/cloud-storage/ai/src/tool/tool_loop/mod.rs
💤 Files with no reviewable changes (4)
- rust/cloud-storage/ai/src/tool/tool_loop/mod.rs
- rust/cloud-storage/ai/src/tool/tool_loop/ai_client.rs
- rust/cloud-storage/ai/src/tool/tool_loop/constant.rs
- rust/cloud-storage/ai/src/tool/tool_loop/chained.rs
Comment on lines
+111
to
+117
| while let Some(item) = stream.next().await { | ||
| if let Err(e) = item { | ||
| yield Err(e); | ||
| break 'outer; | ||
| } | ||
|
|
||
| let part_or_ext = item.unwrap(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using match instead of if let + unwrap().
After checking for Err, the code uses .unwrap() on Line 117. While safe here, this pattern is less idiomatic than using a match or let-else.
Proposed refactor
while let Some(item) = stream.next().await {
- if let Err(e) = item {
- yield Err(e);
- break 'outer;
- }
-
- let part_or_ext = item.unwrap();
+ let part_or_ext = match item {
+ Ok(p) => p,
+ Err(e) => {
+ yield Err(e);
+ break 'outer;
+ }
+ };
match part_or_ext {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` around lines 111 -
117, The loop currently checks for Err with `if let Err(e)` and then calls
`item.unwrap()`, which is non-idiomatic; change the `while let Some(item) =
stream.next().await { ... }` body to use a `match item { Err(e) => { yield
Err(e); break 'outer; }, Ok(part_or_ext) => { /* use part_or_ext here */ } }`
(or a `let Ok(part_or_ext) = item else { yield Err(e); break 'outer; }` style)
so you never call `.unwrap()` and the `part_or_ext` binding is produced directly
and safely in the `agent.rs` loop handling the stream.
Contributor
There was a problem hiding this comment.
@ehayes2000 Understood, I'll drop it! 🐇
(´• ω •`)ᵒ
🧠 Learnings used
Learnt from: CR
Repo: macro-inc/macro PR: 0
File: rust/cloud-storage/CLAUDE.md:0-0
Timestamp: 2026-03-23T19:07:10.919Z
Learning: Applies to rust/cloud-storage/**/*.rs : Prefer using `.inspect_err()` instead of `if let Err(e)` when doing logging in error handling
Learnt from: CR
Repo: macro-inc/macro PR: 0
File: rust/cloud-storage/CLAUDE.md:0-0
Timestamp: 2026-03-23T19:07:10.919Z
Learning: Applies to rust/cloud-storage/**/*.rs : Prefer `anyhow::bail!("error message")` over `Err(anyhow::anyhow!("error message"))` for early returns with errors
Learnt from: CR
Repo: macro-inc/macro PR: 0
File: rust/cloud-storage/CLAUDE.md:0-0
Timestamp: 2026-03-23T19:07:10.919Z
Learning: Applies to rust/cloud-storage/**/*.rs : Include `err` when adding the `tracing::instrument` attribute to functions that return `Result`. Do not include `err` on functions that return `Option`, `()`, or other non-`Result` types. Never include `level = "info"`
Comment on lines
+366
to
+372
| Err(e) => { | ||
| tracing::error!( | ||
| err=?e, | ||
| "ToolCall::try_from failed from {:#?}", | ||
| call | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use consistent error field name in tracing.
The coding guidelines specify using error=?e for error logging, but this uses err=?e. This inconsistency can make log querying harder.
Proposed fix
Err(e) => {
tracing::error!(
- err=?e,
+ error=?e,
"ToolCall::try_from failed from {:#?}",
call
);
}As per coding guidelines: "Log errors as tracing::error!(error=?e, "msg") — do not interpolate errors into the message string."
📝 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.
Suggested change
| Err(e) => { | |
| tracing::error!( | |
| err=?e, | |
| "ToolCall::try_from failed from {:#?}", | |
| call | |
| ); | |
| } | |
| Err(e) => { | |
| tracing::error!( | |
| error=?e, | |
| "ToolCall::try_from failed from {:#?}", | |
| call | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` around lines 366 -
372, In the ToolCall::try_from error branch inside the agent loop, change the
tracing field name from err=?e to error=?e in the tracing::error! invocation so
it follows the project's logging convention (i.e., use tracing::error!(error=?e,
"ToolCall::try_from failed from {:#?}", call)); do not interpolate the error
into the message string.
| } | ||
| } | ||
|
|
||
| // anthropic sometimes doesn't return responses for their servier tools. this causes 400's |
Contributor
There was a problem hiding this comment.
Fix typo in comment: "servier" → "server".
-// anthropic sometimes doesn't return responses for their servier tools. this causes 400's
+// Anthropic sometimes doesn't return responses for their server tools. This causes 400 errors.📝 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.
Suggested change
| // anthropic sometimes doesn't return responses for their servier tools. this causes 400's | |
| // Anthropic sometimes doesn't return responses for their server tools. This causes 400 errors. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` at line 404, Update
the inline comment that currently reads "anthropic sometimes doesn't return
responses for their servier tools. this causes 400's" to correct the typo
"servier" → "server" so it reads "anthropic sometimes doesn't return responses
for their server tools. this causes 400's"; locate and edit the comment in the
agent.rs chat agent code (the comment that begins with "anthropic sometimes...")
to apply this single-word fix.
Comment on lines
+405
to
+406
| #[tracing::instrument(skip_all)] | ||
| fn correct_message_chain( |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding #[must_use] attribute for the pure function.
Since correct_message_chain returns a corrected message list without side effects, adding #[must_use] would prevent accidental ignoring of the return value.
+#[must_use]
#[tracing::instrument(skip_all)]
fn correct_message_chain(📝 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.
Suggested change
| #[tracing::instrument(skip_all)] | |
| fn correct_message_chain( | |
| #[must_use] | |
| #[tracing::instrument(skip_all)] | |
| fn correct_message_chain( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` around lines 405 -
406, Add the #[must_use] attribute to the pure function correct_message_chain so
callers are warned if they ignore its returned corrected message list; place
#[must_use] directly above the function declaration (the existing
#[tracing::instrument(skip_all)] can remain, so add #[must_use] on its own line
immediately above or below the tracing attribute to ensure the compiler enforces
use of the return value from correct_message_chain.
ehayes2000
force-pushed
the
fix-noresponse
branch
from
63f2221 to
d9ad5f3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tool/tool_loop/chat/agent.rs`:
- Around line 183-190: The tail assistant flush is unconditional and can append
an empty synthetic assistant message; fix by guarding both the server-tool
branch (StreamPart::ToolResponse(ToolResponse::Json { .. })) and the final
unconditional flush (around the code currently at lines ~276-278) so you only
call self.make_assistant_message and push its result when there is actual
content to flush: check that content.is_empty() || tool_calls.is_empty() is
false and also that pending_tool_messages is not empty (i.e., only flush when
content.len() > 0 or !tool_calls.is_empty() or
!pending_tool_messages.is_empty()), then append pending_tool_messages and
messages only in that case to avoid creating an empty assistant turn.
- Around line 19-20: The code currently uses HashMap::into_values() to iterate
tool calls, which loses the model-emitted ordering because HashMap is unordered;
locate where into_values() is used for processing/persisting tool calls (the
places handling ToolCall objects and their call.index) and replace the unordered
iteration with an order-preserving step: collect the values into a Vec, sort
that Vec by each call's call.index (or use an insertion-ordered map like
indexmap::IndexMap keyed by call.index), then iterate the sorted/ordered
collection when executing and persisting tool calls so the final execution order
matches the model emission.
- Around line 360-372: The branch handling ToolCall::try_from failure (inside
the FinishReason::ToolCalls handling) currently logs and drops the malformed
call; instead propagate the failure so the stream and conversation state do not
silently continue—when ToolCall::try_from(call.clone()) returns Err(e) yield or
return an error to the caller (e.g., yield Err(...) or return Err(...)) with
contextual information referencing FinishReason::ToolCalls, ToolCall::try_from,
and the StreamPart/PartOrExt flow so consumers observe the provider error and
the loop terminates rather than swallowing the bad call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28dd1615-71dc-48b8-8f0e-11feebcf776b
📒 Files selected for processing (7)
rust/cloud-storage/ai/src/tool/tool_loop/ai_client.rsrust/cloud-storage/ai/src/tool/tool_loop/chained.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/mod.rsrust/cloud-storage/ai/src/tool/tool_loop/chat/test.rsrust/cloud-storage/ai/src/tool/tool_loop/constant.rsrust/cloud-storage/ai/src/tool/tool_loop/mod.rs
💤 Files with no reviewable changes (4)
- rust/cloud-storage/ai/src/tool/tool_loop/mod.rs
- rust/cloud-storage/ai/src/tool/tool_loop/ai_client.rs
- rust/cloud-storage/ai/src/tool/tool_loop/constant.rs
- rust/cloud-storage/ai/src/tool/tool_loop/chained.rs
Comment on lines
+19
to
+20
| use std::collections::{HashMap, HashSet}; | ||
| use std::sync::Arc; |
Contributor
There was a problem hiding this comment.
Preserve the streamed tool-call order.
HashMap::into_values() does not preserve call.index, so multi-tool turns can be executed and persisted in a different order than the model emitted them.
Proposed fix
@@
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeMap, HashMap, HashSet};
@@
- let mut tool_calls: HashMap<u32, PartialToolCall> = HashMap::new();
+ let mut tool_calls: BTreeMap<u32, PartialToolCall> = BTreeMap::new();
@@
- tool_calls = HashMap::new();
+ tool_calls = BTreeMap::new();Also applies to: 312-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` around lines 19 - 20,
The code currently uses HashMap::into_values() to iterate tool calls, which
loses the model-emitted ordering because HashMap is unordered; locate where
into_values() is used for processing/persisting tool calls (the places handling
ToolCall objects and their call.index) and replace the unordered iteration with
an order-preserving step: collect the values into a Vec, sort that Vec by each
call's call.index (or use an insertion-ordered map like indexmap::IndexMap keyed
by call.index), then iterate the sorted/ordered collection when executing and
persisting tool calls so the final execution order matches the model emission.
Comment on lines
+183
to
+190
| StreamPart::ToolResponse(ToolResponse::Json { id, json, .. }) => { | ||
| // Server-side tool response - flush current assistant and start new segment | ||
| if !content.is_empty() || !tool_calls.is_empty() { | ||
| messages.push( | ||
| self.make_assistant_message(&mut content, &mut tool_calls), | ||
| ); | ||
| messages.append(&mut pending_tool_messages); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard the final assistant flush.
If the server-tool branch already flushed the pending assistant/tool-call segment, the unconditional tail flush appends an empty assistant message. That synthetic turn is then kept in self.messages and replayed on the next request.
Proposed fix
@@
- // Flush remaining assistant content
- messages.push(self.make_assistant_message(&mut content, &mut tool_calls));
+ // Flush remaining assistant content
+ if !content.is_empty() || !tool_calls.is_empty() {
+ messages.push(self.make_assistant_message(&mut content, &mut tool_calls));
+ }
messages.append(&mut pending_tool_messages);Also applies to: 276-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` around lines 183 -
190, The tail assistant flush is unconditional and can append an empty synthetic
assistant message; fix by guarding both the server-tool branch
(StreamPart::ToolResponse(ToolResponse::Json { .. })) and the final
unconditional flush (around the code currently at lines ~276-278) so you only
call self.make_assistant_message and push its result when there is actual
content to flush: check that content.is_empty() || tool_calls.is_empty() is
false and also that pending_tool_messages is not empty (i.e., only flush when
content.len() > 0 or !tool_calls.is_empty() or
!pending_tool_messages.is_empty()), then append pending_tool_messages and
messages only in that case to avoid creating an empty assistant turn.
Comment on lines
+360
to
+372
| if let Some(FinishReason::ToolCalls) = first.finish_reason { | ||
| for call in tool_calls.into_values() { | ||
| match ToolCall::try_from(call.clone()) { | ||
| Ok(call) => { | ||
| yield Ok(PartOrExt::Part(StreamPart::ToolCall(call))) | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| err=?e, | ||
| "ToolCall::try_from failed from {:#?}", | ||
| call | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast when a streamed tool call cannot be reconstructed.
This branch only logs and drops the malformed call. The outer loop can then finish as if no tool was requested, which hides a real provider error and leaves the conversation state inconsistent.
Proposed fix
@@
Err(e) => {
tracing::error!(
err=?e,
"ToolCall::try_from failed from {:#?}",
call
);
+ yield Err(e.into());
+ break;
}📝 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.
Suggested change
| if let Some(FinishReason::ToolCalls) = first.finish_reason { | |
| for call in tool_calls.into_values() { | |
| match ToolCall::try_from(call.clone()) { | |
| Ok(call) => { | |
| yield Ok(PartOrExt::Part(StreamPart::ToolCall(call))) | |
| } | |
| Err(e) => { | |
| tracing::error!( | |
| err=?e, | |
| "ToolCall::try_from failed from {:#?}", | |
| call | |
| ); | |
| } | |
| if let Some(FinishReason::ToolCalls) = first.finish_reason { | |
| for call in tool_calls.into_values() { | |
| match ToolCall::try_from(call.clone()) { | |
| Ok(call) => { | |
| yield Ok(PartOrExt::Part(StreamPart::ToolCall(call))) | |
| } | |
| Err(e) => { | |
| tracing::error!( | |
| err=?e, | |
| "ToolCall::try_from failed from {:#?}", | |
| call | |
| ); | |
| yield Err(e.into()); | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/ai/src/tool/tool_loop/chat/agent.rs` around lines 360 -
372, The branch handling ToolCall::try_from failure (inside the
FinishReason::ToolCalls handling) currently logs and drops the malformed call;
instead propagate the failure so the stream and conversation state do not
silently continue—when ToolCall::try_from(call.clone()) returns Err(e) yield or
return an error to the caller (e.g., yield Err(...) or return Err(...)) with
contextual information referencing FinishReason::ToolCalls, ToolCall::try_from,
and the StreamPart/PartOrExt flow so consumers observe the provider error and
the loop terminates rather than swallowing the bad call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
coin flip ass anthropic