fix(bridge): sanitize orphaned tool results in v2 adapter - #1975
Merged
Merged
Conversation
serrrfirat
marked this pull request as ready for review
April 3, 2026 15:29
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a sanitization step for tool messages within the LlmBridgeAdapter by calling sanitize_tool_messages before provider execution. This ensures that orphaned action results are appropriately handled. Additionally, new unit tests have been added to verify the correct processing of both orphaned and matched action results. There are no review comments to address, and I have no further feedback to provide.
ilblackdragon
left a comment
Member
There was a problem hiding this comment.
Automated review — LGTM with minor notes
TL;DR: Well-scoped bug fix. Correctly isolates orphaned tool results at the adapter boundary using the existing sanitize_tool_messages helper. Good test coverage. Ready to merge after addressing the visibility nit below.
Findings
| # | Severity | File:Line | Issue | Suggestion |
|---|---|---|---|---|
| 1 | Low | src/llm/mod.rs:59 |
pub(crate) use sanitize_tool_messages re-export — per CLAUDE.md "No pub use re-exports unless exposing to downstream consumers" |
Use the direct path use crate::llm::provider::sanitize_tool_messages at the sole call site in llm_adapter.rs |
| 2 | Low | src/bridge/llm_adapter.rs:51 |
let mut chat_messages but only mutated once via sanitize |
Either document the in-place rationale or accept &[ChatMessage] and return a new vec |
| 3 | Low | Test structure | All three new async tests use CapturingProvider that always returns Ok(...); no negative path |
Add a test where the provider returns an error after sanitize, to verify error propagation is intact |
Strengths
- Root cause clearly identified (missing
sanitize_tool_messagescall in adapter path → Anthropic error 2013) - Smart reuse of existing helper (DRY)
- Uses
tracing::debug!()— respects the TUI logging convention - Tests cover both
complete()andcomplete_with_tools()paths
Open questions
- Is the
pub(crate)re-export intentional for future callers, or can it stay private to theprovidermodule? - Are there other adapter paths (streaming, retries, failover) that could also receive corrupted message sequences and should call sanitize?
ilblackdragon
approved these changes
Apr 6, 2026
This was referenced Apr 6, 2026
drchirag1991
pushed a commit
to drchirag1991/ironclaw
that referenced
this pull request
Apr 8, 2026
* fix(bridge): sanitize orphaned tool results in v2 adapter * test(bridge): cover no-tool sanitizer path
This was referenced Apr 8, 2026
This was referenced Apr 10, 2026
Merged
Merged
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
* fix(bridge): sanitize orphaned tool results in v2 adapter * test(bridge): cover no-tool sanitizer path
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
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.
Summary
sanitize_tool_messageshelper from the LLM moduleRoot cause
The V2
LlmBridgeAdapterconvertedThreadMessagevalues directly intoChatMessagevalues and forwarded them to the provider without callingsanitize_tool_messages(). When thread history contained an orphaned action result, Anthropic rejected the request with error 2013 because thetool_resultno longer had a matching preceding tool call.Impact
Validation
cargo fmt --all --checkcargo test bridge::llm_adapter::tests --libCloses #1950