chore: unify tool call result into tool dispatch result - #34
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 36 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughConsolidates ToolDispatchResult into core types (replacing ToolCallResult) and rewires middleware and BareLoop: adds a tool dispatch pipeline, recovery, hooks/health recording, emission/observer helpers, message builders, streaming, and context compaction; tests updated accordingly. ChangesTool Dispatch Result Consolidation & Engine Flow
Sequence DiagramsequenceDiagram
participant Client
participant BareLoop
participant ToolPipeline
participant ToolRegistry
participant Reflector
participant Recovery
participant EventSink
Client->>BareLoop: start turn / stream request
BareLoop->>ToolRegistry: lookup tool / pipeline
BareLoop->>ToolPipeline: invoke pipeline.invoke(ctx) or call tool via select!
ToolPipeline-->>BareLoop: ToolOutput / ToolError (with duration)
BareLoop->>Reflector: reflect(error_text) (on failure)
Reflector-->>Recovery: analysis
Recovery-->>BareLoop: RecoveryAction (Retry/Skip/Fail)
BareLoop->>EventSink: emit tool start/complete events (with duration, is_error)
BareLoop-->>Client: return Message, Usage, StreamStopReason
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
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/engine/middleware.rs (1)
487-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
tool_call_idon middleware dispatch results.This function now builds the unified
ToolDispatchResult, but it never copiesctx.call_idinto it.ToolDispatchContext::call_idis the correlation key for mapping the tool response back to the originating model tool call, so leaving it unset will produce uncorrelated results downstream.Suggested fix
fn dispatch( &self, ctx: &mut ToolDispatchContext, ) -> Pin<Box<dyn Future<Output = ToolDispatchResult> + Send + '_>> { let tool_name = ctx.tool_name.clone(); + let call_id = ctx.call_id.clone(); let input = ctx.input.clone(); let tool_ctx = ctx.tool_context.clone(); let registry = Arc::clone(&self.registry); let cancel = Arc::clone(&ctx.cancel); Box::pin(async move { let start = Instant::now(); let Some(tool) = registry.get(&tool_name) else { let available: Vec<String> = registry.tool_names(); let available_refs: Vec<&str> = available.iter().map(String::as_str).collect(); let error = AgentError::tool_not_found(&tool_name, &available_refs); - return ToolDispatchResult::err(&tool_name, error.to_string(), start.elapsed()); + let mut result = + ToolDispatchResult::err(&tool_name, error.to_string(), start.elapsed()); + result.tool_call_id = Some(call_id.clone()); + return result; }; let call_result = tokio::select! { r = tool.call(input, &tool_ctx) => r, () = cancel.notified() => { - return ToolDispatchResult::err( + let mut result = ToolDispatchResult::err( &tool_name, format!("Tool '{tool_name}' cancelled"), start.elapsed(), ); + result.tool_call_id = Some(call_id.clone()); + return result; } }; let duration = start.elapsed(); - ToolDispatchResult::from_result(&tool_name, call_result, duration) + let mut result = ToolDispatchResult::from_result(&tool_name, call_result, duration); + result.tool_call_id = Some(call_id); + result }) }🤖 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/engine/middleware.rs` around lines 487 - 514, The middleware currently never sets the correlation key ctx.call_id on ToolDispatchResult, so capture ctx.call_id (e.g. let call_id = ctx.call_id.clone();) and propagate it into the dispatch result in both error and success paths: when returning ToolDispatchResult::err(...) set the tool_call_id to call_id (either by passing it into the constructor if available or by mutating the returned ToolDispatchResult.tool_call_id = call_id.clone()), and likewise after calling ToolDispatchResult::from_result(&tool_name, call_result, duration) assign its tool_call_id = call_id.clone() before returning; update all returns in this function (the registry-miss error, the cancelled branch, and the normal result path) to include the call_id so downstream code can correlate responses.
🧹 Nitpick comments (1)
src/engine/bare.rs (1)
1812-1814: ⚡ Quick winPreserve pipeline-provided
tool_call_idinstead of overwriting it.
dispatch_via_pipelinecurrently discardsdispatch_result.tool_call_idand always writesSome(tc.id.clone()). Keeping the pipeline value (with fallback) avoids losing middleware-level normalization/rewrite behavior.Suggested patch
- Ok(ToolDispatchResult { - tool_call_id: Some(tc.id.clone()), + Ok(ToolDispatchResult { + tool_call_id: dispatch_result + .tool_call_id + .or_else(|| Some(tc.id.clone())), output: dispatch_result.output, is_error: dispatch_result.is_error, duration: dispatch_result.duration, resolved_tool_name: dispatch_result.resolved_tool_name, })🤖 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/engine/bare.rs` around lines 1812 - 1814, In dispatch_via_pipeline, don't unconditionally overwrite the pipeline-provided tool_call_id; when constructing the Ok(ToolDispatchResult { ... }) use dispatch_result.tool_call_id if it is Some, and only fall back to Some(tc.id.clone()) when dispatch_result.tool_call_id is None — i.e., set tool_call_id = dispatch_result.tool_call_id.or(Some(tc.id.clone())) (referencing dispatch_result, tc, and ToolDispatchResult).
🤖 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/core/types.rs`:
- Around line 619-624: ToolDispatchResult currently allows missing tool_call_id
(pub tool_call_id: Option<String>) and public constructors/From<ToolOutput>
create instances with None, which lets uncorrelated results escape; change the
public API so ToolDispatchResult always contains a concrete call id (make
tool_call_id: String) and make existing constructors/From implementations either
require a call id or be made private/internal, or alternatively introduce an
internal PreDispatchResult (with Option<String>) used inside the dispatch layer
and expose only the correlated ToolDispatchResult via with_call_id; update
with_call_id to consume/produce the non-optional ToolDispatchResult and fix all
From<ToolOutput> and constructors to produce the internal pre-type or accept a
call id, and adjust MessagePart::tool_result usage to assume a present id.
---
Outside diff comments:
In `@src/engine/middleware.rs`:
- Around line 487-514: The middleware currently never sets the correlation key
ctx.call_id on ToolDispatchResult, so capture ctx.call_id (e.g. let call_id =
ctx.call_id.clone();) and propagate it into the dispatch result in both error
and success paths: when returning ToolDispatchResult::err(...) set the
tool_call_id to call_id (either by passing it into the constructor if available
or by mutating the returned ToolDispatchResult.tool_call_id = call_id.clone()),
and likewise after calling ToolDispatchResult::from_result(&tool_name,
call_result, duration) assign its tool_call_id = call_id.clone() before
returning; update all returns in this function (the registry-miss error, the
cancelled branch, and the normal result path) to include the call_id so
downstream code can correlate responses.
---
Nitpick comments:
In `@src/engine/bare.rs`:
- Around line 1812-1814: In dispatch_via_pipeline, don't unconditionally
overwrite the pipeline-provided tool_call_id; when constructing the
Ok(ToolDispatchResult { ... }) use dispatch_result.tool_call_id if it is Some,
and only fall back to Some(tc.id.clone()) when dispatch_result.tool_call_id is
None — i.e., set tool_call_id =
dispatch_result.tool_call_id.or(Some(tc.id.clone())) (referencing
dispatch_result, tc, and ToolDispatchResult).
🪄 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: 7fc27225-60c4-4513-8993-d93783f9fcb6
📒 Files selected for processing (4)
src/core.rssrc/core/types.rssrc/engine/bare.rssrc/engine/middleware.rs
Comment on lines
+619
to
+624
| /// The tool call this result is for. | ||
| /// | ||
| /// Matches [`ToolCall::id`] to correlate results back to their | ||
| /// originating requests. | ||
| pub tool_call_id: String, | ||
| /// originating requests. Set by the engine after the middleware | ||
| /// pipeline completes via [`with_call_id`](Self::with_call_id). | ||
| pub tool_call_id: Option<String>, |
Contributor
There was a problem hiding this comment.
Don't let public dispatch results escape without a call ID.
ToolDispatchResult now models tool_call_id as optional, and the public constructors/From<ToolOutput> all create instances with None. Downstream, MessagePart::tool_result still requires a concrete call ID (src/message.rs:522-532), and this PR’s engine path falls back to unwrap_or_default() when converting these results back into tool-result messages. A missed with_call_id(...) therefore becomes an empty ID instead of preserving ToolCall::id, which breaks result correlation and can produce invalid tool-result messages. Consider keeping the public type fully correlated, or introducing an internal pre-correlation builder so incomplete results cannot escape the dispatch layer.
Also applies to: 661-684, 776-784
🤖 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/types.rs` around lines 619 - 624, ToolDispatchResult currently
allows missing tool_call_id (pub tool_call_id: Option<String>) and public
constructors/From<ToolOutput> create instances with None, which lets
uncorrelated results escape; change the public API so ToolDispatchResult always
contains a concrete call id (make tool_call_id: String) and make existing
constructors/From implementations either require a call id or be made
private/internal, or alternatively introduce an internal PreDispatchResult (with
Option<String>) used inside the dispatch layer and expose only the correlated
ToolDispatchResult via with_call_id; update with_call_id to consume/produce the
non-optional ToolDispatchResult and fix all From<ToolOutput> and constructors to
produce the internal pre-type or accept a call id, and adjust
MessagePart::tool_result usage to assume a present id.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/engine/bare/dispatch.rs (1)
404-410: 💤 Low valueSimplify the rebuild and fix the misleading doc.
Since the pipeline already returns a
ToolDispatchResult, this manual field-by-field copy can be replaced withwith_call_id. The method doc (Lines 332-335) also reads "converts theToolDispatchResultback to aToolDispatchResult", a leftover from the rename.♻️ Use the builder
- Ok(ToolDispatchResult { - tool_call_id: tc.id.clone(), - output: dispatch_result.output, - is_error: dispatch_result.is_error, - duration: dispatch_result.duration, - resolved_tool_name: dispatch_result.resolved_tool_name, - }) + Ok(dispatch_result.with_call_id(tc.id.clone()))🤖 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/engine/bare/dispatch.rs` around lines 404 - 410, Replace the manual field-by-field reconstruction of ToolDispatchResult with the provided helper method: call dispatch_result.with_call_id(tc.id.clone()) instead of building Ok(ToolDispatchResult { .. }) and remove the misleading doc phrase about "converts the `ToolDispatchResult` back to a `ToolDispatchResult` in the with_call_id/doc comment (update the doc to state it attaches/sets the call id on an existing ToolDispatchResult). Ensure you reference the existing symbols dispatch_result, tc.id, ToolDispatchResult, and with_call_id when making the change.
🤖 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/engine/bare/dispatch.rs`:
- Around line 287-290: The post-tool-use hook is discarding multipart outputs
because the match in the block that builds output_text returns String::new() for
ToolContent::Multipart; change that to flatten/join the multipart pieces to text
the same way as dispatch_via_pipeline and the direct path (i.e. use the existing
text_content() helper or join the parts) so hooks (audit/policy) receive the
full text; consider extracting a shared ToolContent->String helper (e.g.,
ToolContent::text_content or a free function) and use it here instead of
special-casing Multipart.
In `@src/engine/bare/stream.rs`:
- Line 75: The call drop(accumulator.process(&event)) discards the Result from
StreamAccumulator::process(&StreamEvent) and hides
StreamError::InvalidToolInputJson raised at StreamEvent::PartStop; change the
call to propagate the Result instead (e.g., use accumulator.process(&event)? or
map_err/return Err(...)) so the caller returns an error instead of ignoring it;
also update any callers that previously ignored this Result (e.g., places using
.ok() in the stream handler) to propagate or handle the error consistently.
---
Nitpick comments:
In `@src/engine/bare/dispatch.rs`:
- Around line 404-410: Replace the manual field-by-field reconstruction of
ToolDispatchResult with the provided helper method: call
dispatch_result.with_call_id(tc.id.clone()) instead of building
Ok(ToolDispatchResult { .. }) and remove the misleading doc phrase about
"converts the `ToolDispatchResult` back to a `ToolDispatchResult` in the
with_call_id/doc comment (update the doc to state it attaches/sets the call id
on an existing ToolDispatchResult). Ensure you reference the existing symbols
dispatch_result, tc.id, ToolDispatchResult, and with_call_id when making the
change.
🪄 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: 0ba39205-2137-4cb6-b8e0-f716da4a426f
📒 Files selected for processing (7)
src/core/types.rssrc/engine/bare.rssrc/engine/bare/compact.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/bare/message.rssrc/engine/bare/stream.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engine/bare.rs
This was referenced Jul 13, 2026
Merged
Merged
Merged
bobrykov
added a commit
that referenced
this pull request
Aug 18, 2026
chore: unify tool call result into tool dispatch result
Merged
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.
No description provided.