feat(tools): give tool results an image channel and add view_image - #843
feat(tools): give tool results an image channel and add view_image#843Vasanthdev2004 wants to merge 3 commits into
Conversation
Closes #824. tools.Result carried only text, so no tool could hand the model a picture. The sharpest edge was our own capture tooling: browser_screenshot, desktop_screenshot and browser_pdf wrote a PNG, set Meta["artifact_path"] and returned "Artifact captured: <path>". Nothing read that path afterwards, so the model asked for a screenshot, was told one existed, and could not look at it. Zero was never image-blind on the way in — zeroruntime.ImageBlock, Message.Images and internal/imageinput are mature. What was missing is the model-initiated leg. Result.Images is delivered as a following USER message, not on the tool result itself. Every provider drops images on a tool-role message: Anthropic maps a tool result to a tool_result block whose content is a string, Gemini to a functionResponse, and OpenAI guards its image content-parts to the user role with an explicit comment saying so. Attaching them to the tool result would have plumbed the whole path and delivered nothing. A separate message also keeps one tool result per tool call, which the providers validate. Blocks with no data, or a media type outside the provider allow-list, are dropped rather than sent — a rejected image would fail the whole turn, and the tool's text still stands on its own. The message names the tool, since the images arrive detached from their result and nothing else says which call produced them when several ran in one turn. view_image resolves through resolveScopedReadPath before imageinput.LoadFile. That scoping is load-bearing: LoadFile joins a relative path to the workspace root and otherwise takes the path as given, so without it the tool would read anywhere on disk. It is registered with the other scoped readers and declared read-only and thread-safe, matching read_file. The capture tools now attach what they wrote. A PDF or an unreadable file yields no image and the text stands alone, so a capture is never failed over its attachment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds the ChangesTool Image Delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Tool
participant tools.Result
participant AgentLoop
participant Model
Tool->>tools.Result: return image block
tools.Result->>AgentLoop: provide tool result and Images
AgentLoop->>AgentLoop: validate, copy, and normalize images
AgentLoop->>Model: send tool results, then user-role image message
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3
🤖 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 `@internal/agent/loop.go`:
- Around line 712-721: Update the tool-result processing loop in the surrounding
function to collect image messages instead of appending them immediately after
each result. Append the collected images only after every tool result has been
recorded, including aborted-call placeholders on early exits, so all tool
results remain contiguous. Add a regression test covering two tool calls where
the first returns an image.
In `@internal/specialist/manifest.go`:
- Around line 85-86: Update the tool manifest’s execute category to include
view_image alongside the existing inspection tools, then add a ResolveTools
regression test that verifies a specialist configured with tools: [execute]
receives view_image.
In `@internal/tools/registry_test.go`:
- Around line 33-34: Extend the toolset validation test around the existing
tool-count assertion and loop to verify that seen[ViewImageToolName] is
registered after iteration. Keep the count check intact while adding this
regression assertion for the specific view-image tool name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db56b8c4-ec8a-4fbb-9a12-828b0ea0f92f
📒 Files selected for processing (12)
internal/agent/loop.gointernal/agent/tool_result_images_test.gointernal/agent/types.gointernal/specialist/exec.gointernal/specialist/exec_test.gointernal/specialist/manifest.gointernal/tools/local_capture.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/types.gointernal/tools/view_image.gointernal/tools/view_image_test.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
The image messages are user-role, and the first version appended each one
immediately after its own tool result. With two tool calls in a turn that
produces tool_result, user, tool_result — and a user message between two
tool results breaks strict provider replay.
Anthropic makes it concrete. appendUserBlocks coalesces consecutive user
content into one block list, and tool_result blocks must come first in that
message, so the interleaved shape maps to:
[tool_result, text, image, tool_result]
which the API rejects. loop.go already documents this exact rule three lines
below the change, for the self-correction feedback it defers for the same
reason.
Images are now collected during the tool loop and appended once every
tool_result is recorded — including the aborted placeholders on the abort,
stop-reason and repeated-failure paths, so an early exit cannot strand them
mid-sequence either.
The original test used a single tool call, which cannot exhibit the bug. The
new one runs two calls where the first returns an image and asserts the image
message does not fall between the first and last tool result.
Also from review: view_image joins the "execute" tool category, which grants
read_file but not view_image, so a specialist declared with tools: [execute]
could not look at a screenshot it had just captured. Covered by a test that
holds for every category granting read_file rather than naming one. And the
core read-only set is now asserted by tool name, not only by count — a count
still passes when one reader is swapped for another.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All three fixed on Tool-result contiguity. Correct, and my implementation was wrong. Images are user-role messages and I appended each one immediately after its own tool result, so two tool calls produced
Images are now collected during the loop and flushed once every The reason this shipped: my test used one tool call, which cannot exhibit the bug. The new test runs two calls with the image on the first and asserts the image message doesn't fall between the first and last tool result. Reverting to the old append fails it.
By-name registry assertion. Added. A count assertion passes just as happily when one reader is swapped for another. Four mutations, all killed: reverting to the immediate append, never flushing, dropping |
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 `@internal/agent/tool_result_images_test.go`:
- Around line 142-165: Strengthen the assertions in the result message
validation loop to collect each tool-result index and ID, then require exactly
two results in order matching c1 and c2 with adjacent indexes. Require the user
image message to occur immediately after the second tool result, replacing the
current weaker firstTool/lastTool and relative-position checks, and add a
regression test covering a non-image message between the tool results.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6245c21-ea95-4cb7-a069-bf12504bd62b
📒 Files selected for processing (5)
internal/agent/loop.gointernal/agent/tool_result_images_test.gointernal/specialist/manifest.gointernal/specialist/view_image_category_test.gointernal/tools/registry_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/specialist/manifest.go
- internal/tools/registry_test.go
- internal/agent/loop.go
The assertions only tracked the first and last tool-result index, so four distinct broken orderings passed: a message wedged between the two tool results, a duplicated image flush, a duplicated tool result, and both results answering the same tool_call_id. Mutation-testing each of those against the real loop confirmed the old assertions were green on all four. Now it collects every tool-result index and id and requires exactly two, in c1-then-c2 order, at adjacent indexes, with the image following the last one and delivered exactly once. Adjacency is the assertion that carries the contract: Anthropic needs every tool_result block ahead of any other block in the user message they coalesce into, so anything between the two results is the 400 this change exists to avoid. The image is deliberately NOT pinned to exactly last+1. appendUserBlocks merges consecutive user-role content, so [tool_result, tool_result, text, image] maps to tool_result blocks first and replays fine; pinning would fail that valid shape while killing nothing the adjacency and count assertions do not already catch. messageShape reports the actual role ordering on failure, and the loop uses two ifs rather than a switch so an image left on a tool-role message is reported as the misplacement it is instead of "the image never reached the model".
e2143ec to
83f3ffa
Compare
1 participant
Closes #824.
tools.Resultcarried only text, so no tool could hand the model a picture. The sharpest edge was our own capture tooling —browser_screenshot,desktop_screenshotandbrowser_pdfwrote a PNG, setMeta["artifact_path"], and returned "Artifact captured:<path>". Nothing read that path afterwards, so the model asked for a screenshot, was told one existed, and couldn't look at it.Zero was never image-blind on the way in —
zeroruntime.ImageBlock,Message.Imagesandinternal/imageinputare all mature. What was missing is the model-initiated leg.The one design decision worth reviewing
Images ride a following user message, not the tool result. I tried the obvious thing first and checked the providers before building on it. All three drop images on a tool-role message:
tool_resultblock whosecontentis a stringfunctionResponsewithResponse: {"result": content}So attaching them to the tool result would have plumbed the entire path and delivered nothing, silently. The separate user message is also the only shape that keeps one tool result per tool call, which the providers validate — I didn't want to solve this by duplicating tool results.
If anyone would rather teach the Anthropic mapper to put an image block inside
tool_result(it does support that), that's a fine follow-up — but OpenAI still needs the user-message path, so the fallback has to exist regardless.The rest
view_imageresolves throughresolveScopedReadPathbeforeimageinput.LoadFile. That scoping is load-bearing, not decoration:LoadFilejoins a relative path to the workspace root and otherwise takes the path as given, so without it the tool reads anywhere on disk. There's a test for the absolute-path and..cases, and deleting the scoping fails it. Registered with the other scoped readers, declared read-only and thread-safe likeread_file, and added to the specialist read-only sets — it's exactly as safe asread_file, which is already in each.Capture tools attach what they wrote. A PDF or unreadable file yields no image and the text stands alone, so a capture is never failed over its attachment.
Empty or disallowed images are dropped rather than sent. A media type outside the provider allow-list would fail the whole turn; the tool's text is still useful without it.
Verified
TestRunDeliversToolResultImagesToTheModelgoes through the realRunloop and asserts the image reaches a user-role message and that the tool-result count is unchanged. Four mutations, all killed: dropping the emit, moving it to the tool role, not carrying images off the result, and removingview_image's path scoping.internal/tools,internal/agentandinternal/specialistgreen; full suite clean apart fromTestBuildServeScopeKeepsLexicalPathsandTestAltScreenTranscriptScrollKeepsFooterFixed, which fail identically on cleanmainon my Windows box.One thing I noticed and deliberately did not change: a missing file surfaces the resolved absolute path in the error, because
resolveScopedReadPathreturns the raw syscall error.read_filedoes the same, so it's a pre-existing convention rather than something this adds — but it does put the user's home directory into model-visible output, and might be worth tightening repo-wide separately.This also unblocks #823 — MCP image content blocks now have somewhere to go.
Summary by CodeRabbit
New Features
view_imagetool for loading supported images within the workspace, including media metadata.Bug Fixes
Tests