Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a private helper that centralizes message construction, resolves Changes
Sequence DiagramsequenceDiagram
participant AgentLoop as Agent Loop
participant Builder as build_resolved_messages
participant ImageResolver as resolve_images_to_base64
participant Provider as LLM Provider
AgentLoop->>Builder: request resolved messages (session, memory_override?)
activate Builder
Builder->>Builder: build messages with memory override
Builder->>ImageResolver: async resolve FilePath images -> base64
activate ImageResolver
ImageResolver-->>Builder: messages with inline/base64 images
deactivate ImageResolver
Builder->>Builder: filter empty user messages (post-resolution)
Builder-->>AgentLoop: resolved & filtered messages
deactivate Builder
AgentLoop->>Provider: send resolved messages
activate Provider
Provider-->>AgentLoop: LLM response
deactivate Provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
rafaellin
changed the title
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/loop.rs`:
- Around line 1482-1497: The current message filtering (using Role::User and
has_images()) runs before resolve_images_to_base64, so unreadable images may be
dropped and leave empty user messages; after calling
resolve_images_to_base64(&mut msgs, dir) re-run the same filter (e.g., retain or
a second filter) on msgs to remove any Role::User entries with empty content and
no images so the final messages Vec (used later) contains no blank user turns;
locate this around build_messages_with_memory_override -> msgs and
resolve_images_to_base64 in loop.rs and apply the additional filtering step.
- Around line 1493-1495: The call to resolve_images_to_base64(&mut msgs, dir)
blocks the Tokio runtime because it does sync fs reads; change this to run on a
blocking thread: refactor resolve_images_to_base64 (or add a new function e.g.,
resolve_images_to_base64_blocking) to accept owned messages (or return an owned
Vec) and perform the fs reads synchronously, then from process_message and
process_message_streaming replace the direct call with
tokio::task::spawn_blocking(move || resolve_images_to_base64_blocking(msgs,
dir)).await?? to get back the processed msgs and assign them into msgs; ensure
you stop taking an &mut msgs reference across the await by moving/returning
ownership and keep using session_manager.sessions_dir() to supply dir.
Owner
ReviewThe core fix is correct — images need to survive filtering and be resolved to base64 for provider compatibility. A couple of things to address before merge: 1. Filter ordering bug (agrees with CodeRabbit)The filter Fix: move the filter to after resolution at all 4 call sites: resolve_images_to_base64(&mut msgs, dir);
msgs.retain(|m| !(m.role == Role::User && m.content.is_empty() && !m.has_images()));2. Code duplicationThe same 12-line block (build messages → filter → resolve → collect) is copy-pasted 4 times. Consider extracting into a helper method on fn build_resolved_messages(&self, session: &Session, memory_override: Option<&str>) -> Vec<Message> { ... }3. Missing linked issuePR template has |
Contributor
Author
|
Closed #368 |
When the agent executes tools in a conversation with images, image file paths were not being resolved to base64 in subsequent LLM API calls during the tool execution loop. This caused the LLM to receive file path references instead of actual image data. This fix: - Adds build_resolved_messages() helper to centralize message preparation - Resolves image paths to base64 before each tool loop LLM call - Filters empty messages AFTER resolution (if image load fails) - Applies fix to all 4 tool loop locations (streaming + non-streaming) Fixes: Images now persist through entire tool execution loops
rafaellin
force-pushed
the
fix/resolve-images-in-tool-loops
branch
from
3064077 to
10aab9c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/loop.rs (1)
2404-2429: Consider reusing this helper for the initial provider calls too.
process_messageandprocess_message_streamingstill duplicate message build/resolve logic before the firstprovider.chat(...). Routing those throughbuild_resolved_messageswould keep one canonical behavior and prevent future drift.♻️ Suggested simplification
- let mut messages = self.context_builder.build_messages_with_memory_override( - &session.messages, - "", - memory_override.as_deref(), - ); - if let Some(dir) = self.session_manager.sessions_dir() { - resolve_images_to_base64(&mut messages, dir); - } + let messages = self.build_resolved_messages(&session, memory_override.as_deref());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/loop.rs` around lines 2404 - 2429, The duplicate message-building and image-resolution logic in process_message and process_message_streaming should be replaced by a call to build_resolved_messages to centralize behavior: in both functions, remove the local build_messages_with_memory_override + resolve_images_to_base64 + retain filter logic and instead call self.build_resolved_messages(&session, memory_override) to obtain msgs before the first provider.chat(...) call; ensure you pass the same session value and memory_override used previously and that build_resolved_messages still uses self.session_manager.sessions_dir() for image resolution so semantics remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/agent/loop.rs`:
- Around line 2404-2429: The duplicate message-building and image-resolution
logic in process_message and process_message_streaming should be replaced by a
call to build_resolved_messages to centralize behavior: in both functions,
remove the local build_messages_with_memory_override + resolve_images_to_base64
+ retain filter logic and instead call self.build_resolved_messages(&session,
memory_override) to obtain msgs before the first provider.chat(...) call; ensure
you pass the same session value and memory_override used previously and that
build_resolved_messages still uses self.session_manager.sessions_dir() for image
resolution so semantics remain identical.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/loop.rs (1)
4041-4122: Add one helper-level regression test for the exact blank-turn scenario.Current tests validate
resolve_images_to_base64, but they don’t directly assertbuild_resolved_messagespost-resolution filtering behavior (the logic used by all call paths). A focused test here would lock in the PR’s core fix and prevent reintroducing filter-order regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/loop.rs` around lines 4041 - 4122, Add a focused regression test that ensures build_resolved_messages drops an entire blank turn created when resolve_images_to_base64 removes unreadable image parts: create a Message containing only a FilePath image pointing at a missing file, call resolve_images_to_base64(&mut messages, tmp.path()).await, then call build_resolved_messages or the public API that uses it and assert the resulting resolved message list does not include an empty/blank turn (e.g., length is 0 or the message was removed); reference the resolve_images_to_base64 and build_resolved_messages helper names so the test validates the exact blank-turn scenario fixed by the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/agent/loop.rs`:
- Around line 4041-4122: Add a focused regression test that ensures
build_resolved_messages drops an entire blank turn created when
resolve_images_to_base64 removes unreadable image parts: create a Message
containing only a FilePath image pointing at a missing file, call
resolve_images_to_base64(&mut messages, tmp.path()).await, then call
build_resolved_messages or the public API that uses it and assert the resulting
resolved message list does not include an empty/blank turn (e.g., length is 0 or
the message was removed); reference the resolve_images_to_base64 and
build_resolved_messages helper names so the test validates the exact blank-turn
scenario fixed by the PR.
taqtiqa-mark
pushed a commit
to taqtiqa-mark/zeptoclaw
that referenced
this pull request
Mar 25, 2026
## Summary <!-- 1-3 bullet points describing what this PR does --> ## Related Issue <!-- Link the issue this PR addresses. Use "Closes #N" to auto-close on merge. --> <!-- If no issue exists, explain why (e.g. typo fix, trivial refactor). --> Closes # ## Scope - [ ] I branched from `upstream/main` - [ ] This PR contains only commits related to this change - [ ] `cargo test` passes - [ ] `cargo clippy -- -D warnings` passes - [ ] `cargo fmt --check` passes ## Test Plan <!-- How did you verify this works? --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Images are reliably converted to inline/base64 before processing, preventing loss across interaction modes. * Empty user messages that become meaningful after image resolution are preserved rather than dropped. * Message preparation is now consistent across initial, streaming, tool-loop, and final/synthesis paths. * **Refactor** * Centralized message preprocessing to ensure uniform behavior and simplify maintenance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
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
2 participants
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
Related Issue
Closes #
Scope
upstream/maincargo testpassescargo clippy -- -D warningspassescargo fmt --checkpassesTest Plan
Summary by CodeRabbit
Bug Fixes
Refactor