Image (multimodal) input: exec --image, stream-json images, TUI /image across all 3 providers#143
Merged
Conversation
Stream-json message images were parsed and validated but never reached agent.Options.Images: streamjson.ResolveImages had no production caller, so images sent over stream-json input were silently dropped. resolveExecPrompt now also returns the resolved stream-json images (nil for text input and for image-free stream-json input). The exec runner merges them with --image attachments before the vision gate, so both input sources flow through the same drop+warn and the same agent.Options.Images wiring. Adds an exec-level end-to-end test that feeds a base64 image on a stream-json message event via stdin on a vision model and asserts the image reaches the agent run (the provider request's user turn).
A user could attach an image on a vision model and then switch to a non-vision model via /model before submitting; the images were threaded unconditionally into the agent turn, reaching a model that rejects them. The commandPrompt submit path now re-runs the vision check against the current effective model. When the active model can't accept images, the pending images are dropped (not threaded) and an inline notice mirroring the headless drop+warn wording is appended. Pending image state is still cleared after submit either way. Adds a test that attaches on a vision model, switches the model id to a non-vision one, submits, and asserts the run received no images, an inline notice was appended, and pending state was cleared.
LoadFile read the whole file into memory before checking the size cap, so a multi-gigabyte file allocated a huge buffer only to be discarded. Add an os.Stat size pre-check before os.ReadFile: a file whose size exceeds MaxImageBytes is rejected with the same 10 MiB-limit error before any read; a missing file surfaces the same not-found error. The existing post-read len(data) check is kept as a TOCTOU backstop in case the file grows between Stat and ReadFile.
The Anthropic and Gemini providers only attach images in their user branches, but OpenAI funnels every role through a single mapMessage, which would emit image content-parts for any role carrying Images. Guard the content-parts path to the user role: a non-user message that happens to carry Images keeps the plain string/nil content path. Pins the invariant with TestMapMessageNonUserRolesNeverCarryImages, asserting assistant/tool/system messages with Images set still serialize plain content (no content-parts array).
gnanam1990
requested review from
Vasanthdev2004 and
anandh8x
and removed request for
Vasanthdev2004
# Conflicts: # internal/cli/exec.go
Contributor
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. |
WalkthroughSeeds image blocks into the initial user message, deep-clones image bytes for history, adds file and stream-json image inputs with size/type validation, gates usage by model vision capability, maps images into OpenAI/Gemini/Anthropic request formats, and updates CLI/TUI wiring and rendering with tests. ChangesMulti-provider vision model image input support
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_TUI as CLI/TUI
participant StreamJSON
participant ImageLoader as imageinput.LoadFile
participant Registry
participant Agent
participant Provider
User->>CLI_TUI: submit prompt (+ optional --image / /image / stream-json images)
CLI_TUI->>ImageLoader: Load file paths
CLI_TUI->>StreamJSON: Parse stdin events (when used)
CLI_TUI->>Registry: SupportsVision(effectiveModel)
CLI_TUI->>Agent: run with Options.Images (SeedMessagesWithImages)
Agent->>Provider: provider.StreamCompletion with mapped multimodal messages
Provider-->>Agent: stream events / completion
Agent-->>CLI_TUI: outputs and transcript updates (image chips cleared)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/tui/model.go (1)
903-909:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPending images are not cleared on a failed submit path (
No provider configured).On Line 903, the early return skips the new clear block (Lines 949-950). That leaves staged images in memory and can unintentionally attach them to a later prompt after provider setup.
Proposed fix
case commandPrompt: m.showSplash = false m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendUser, text: command.text}) + // A prompt submission consumes staged images regardless of run outcome. + turnImages := m.pendingImages + m.pendingImages = nil + m.pendingImageLabels = nil + if m.provider == nil { m.transcript = reduceTranscript(m.transcript, transcriptAction{ kind: actionAppendAssistant, text: "No provider configured.", }) return m, nil } @@ - turnImages := m.pendingImages if len(turnImages) > 0 && !modelSupportsVisionTUI(m.modelName) { @@ - m.pendingImages = nil - m.pendingImageLabels = nil runCtx, cancel := context.WithCancel(m.ctx)Also applies to: 931-950
🤖 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 `@internal/tui/model.go` around lines 903 - 909, When bailing out because m.provider == nil in the submit path, clear any staged/pending image state before returning so images aren't retained for future prompts; specifically, in the early-return block around the reduceTranscript call (where transcriptAction/actionAppendAssistant is used) set the staged image field(s) (e.g., m.stagedImages = nil or make empty) and any related pending image fields to an empty slice or nil, then proceed with the existing reduceTranscript and return; apply the same clear-before-return fix to the other early-return branch around lines 931-950.internal/streamjson/streamjson.go (1)
190-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImage-only message flow still fails at prompt resolution.
Allowing empty
contentwhen images are present (Line 257) conflicts withResolvePrompt, which still errors when no non-empty text exists. That breaks valid image-only message events during stream-json execution.💡 Proposed fix
func ResolvePrompt(events []InputEvent) (string, error) { parts := []string{} + hasImages := false for _, event := range events { + if len(event.Images) > 0 { + hasImages = true + } content := strings.TrimSpace(event.Content) if content != "" { parts = append(parts, content) } } if len(parts) == 0 { + if hasImages { + return "", nil + } return "", ProtocolError{"Stream-json input must include at least one prompt or user message event."} } return strings.Join(parts, "\n\n"), nil }Also applies to: 257-258
🤖 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 `@internal/streamjson/streamjson.go` around lines 190 - 202, ResolvePrompt currently errors if no non-empty text parts are found; update it to permit image-only events by checking InputEvent for image fields (e.g., event.Image, event.Images, or similar image-related properties used in stream-json) — if parts is empty but any event contains an image, return "" with nil error instead of ProtocolError. Modify ResolvePrompt (and any callers if needed) so it treats presence of images as a valid input, referencing the ResolvePrompt function and the InputEvent struct's content and image fields to locate the change.
🧹 Nitpick comments (1)
internal/streamjson/streamjson_test.go (1)
219-241: ⚡ Quick winAdd a regression test for image-only
ResolvePromptbehavior.Current coverage validates parsing image-only messages but does not assert the prompt-resolution step for the same input shape. Please add a test that parses an image-only message and verifies
ResolvePromptbehavior explicitly.Also applies to: 303-342
🤖 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 `@internal/streamjson/streamjson_test.go` around lines 219 - 241, Extend the existing image-only test to also exercise prompt resolution: after calling ParseInput on the image-only JSON (the existing variable imageOnly), call ResolvePrompt on the returned events (or the first event) and assert the behavior you expect for image-only messages (e.g., that ResolvePrompt returns a non-empty resolved prompt structure or an error-free result and preserves the image in Events[0].Images). Update TestValidateInputEventAllowsImageOnlyMessage (or add a new test like TestResolvePromptAllowsImageOnlyMessage) to include the ParseInput -> ResolvePrompt sequence and corresponding assertions referencing ParseInput and ResolvePrompt to ensure the regression is covered.
🤖 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`:
- Line 54: copyMessages currently only deep-copies ToolCalls but leaves
Message.Images and ImageBlock.Data aliased, which causes binary image payloads
seeded by SeedMessagesWithImages to be mutated across copies; update the
copyMessages routine to also deep-copy the Images slice for each Message and
allocate new ImageBlock instances copying the Data byte slices (make a new
[]byte and copy contents) so Message.Images and each ImageBlock.Data are fully
independent across cloned messages.
In `@internal/cli/exec_parse.go`:
- Around line 69-70: The current argument parsing case that appends to
options.imagePaths accepts `--image=` with an empty value; modify the parsing
branch (the case handling strings.HasPrefix(arg, "--image=")) to extract the
value via strings.TrimSpace(strings.TrimPrefix(arg, "--image=")) and if the
resulting value is empty, return a parse error (or propagate an error)
indicating the `--image` flag requires a non-empty value instead of appending it
to options.imagePaths; otherwise append the non-empty value as before.
In `@internal/cli/exec.go`:
- Around line 482-490: Resolve images before resolving the prompt: call
streamjson.ResolveImages(events) first and capture streamImages, then call
streamjson.ResolvePrompt(events). If ResolveImages returns an error, return it
wrapped as execUsageError as before; if ResolvePrompt returns an error but
streamImages is non-empty (i.e., valid image-only input), return an empty prompt
string with the resolved streamImages instead of failing; otherwise propagate
the ResolvePrompt error via execUsageError. Update the variables prompt,
streamImages and error handling around ResolveImages/ResolvePrompt accordingly
(references: ResolveImages, ResolvePrompt, streamImages, prompt,
execUsageError).
In `@internal/imageinput/imageinput.go`:
- Around line 43-51: The current os.ReadFile(resolved) performs an unbounded
read and can allocate beyond MaxImageBytes before the later length check;
replace it with an explicit bounded read using os.Open(resolved) and
io.LimitReader (or io.ReadAll on an io.LimitReader) to read at most
MaxImageBytes+1 bytes, then error if len(data) > MaxImageBytes; update the code
paths that return zeroruntime.ImageBlock{} and the error messages (referencing
variables resolved, path, MaxImageBytes and the ImageBlock return) and ensure
the file is closed (defer f.Close()) and any read errors are propagated instead
of the current os.ReadFile error text.
In `@internal/streamjson/streamjson.go`:
- Around line 215-221: Reject oversized base64 payloads before allocating
decoded bytes by using base64.StdEncoding.DecodedLen on len(image.Data) and
comparing that to maxStreamImageBytes; if it exceeds the limit return the same
ProtocolError, otherwise allocate a buffer of that decoded length and decode
into it (replacing base64.StdEncoding.DecodeString(image.Data)). Update the
logic around base64.StdEncoding.DecodeString, image.Data, and
maxStreamImageBytes in the function so the size check happens prior to decoding
and preserve the existing ProtocolError messages.
In `@internal/zeroline/render.go`:
- Around line 594-597: RenderChat's cmdRegion can produce two rows when
d.ImageChips is non-empty, but the body/overlay height math still assumes a
single-row cmd region, causing a one-row overflow; update the height accounting
in RenderChat (and the similar block around the other cmdRegion usage) to
compute cmdRows = 1 (or 2 when d.ImageChips != "") and subtract cmdRows from the
available height instead of a fixed value (e.g., replace the h - 3 assumption
with h - (baseFooterRows + cmdRows) or adjust by one when ImageChips != ""), and
ensure any calls that build the cmdRegion (the branch using
s.mute.Render(d.ImageChips) and padRight) are consistent with the new cmdRows
calculation so body/overlay sizing matches actual emitted rows.
In `@internal/zeroruntime/helpers.go`:
- Around line 43-47: SeedMessagesWithImages currently aliases the caller-owned
images slice into the returned Message (Message.Images = images); instead
deep-copy the slice and each ImageBlock so the returned Message.Images does not
share memory with the caller. Specifically, in SeedMessagesWithImages make a new
slice of ImageBlock of the same length, copy each ImageBlock value, and for each
ImageBlock copy the Data byte slice into a new byte slice (copying the bytes)
before assigning into the new slice; then set Message.Images to that new slice.
This prevents mutations of ImageBlock.Data by the caller from affecting seeded
messages.
---
Outside diff comments:
In `@internal/streamjson/streamjson.go`:
- Around line 190-202: ResolvePrompt currently errors if no non-empty text parts
are found; update it to permit image-only events by checking InputEvent for
image fields (e.g., event.Image, event.Images, or similar image-related
properties used in stream-json) — if parts is empty but any event contains an
image, return "" with nil error instead of ProtocolError. Modify ResolvePrompt
(and any callers if needed) so it treats presence of images as a valid input,
referencing the ResolvePrompt function and the InputEvent struct's content and
image fields to locate the change.
In `@internal/tui/model.go`:
- Around line 903-909: When bailing out because m.provider == nil in the submit
path, clear any staged/pending image state before returning so images aren't
retained for future prompts; specifically, in the early-return block around the
reduceTranscript call (where transcriptAction/actionAppendAssistant is used) set
the staged image field(s) (e.g., m.stagedImages = nil or make empty) and any
related pending image fields to an empty slice or nil, then proceed with the
existing reduceTranscript and return; apply the same clear-before-return fix to
the other early-return branch around lines 931-950.
---
Nitpick comments:
In `@internal/streamjson/streamjson_test.go`:
- Around line 219-241: Extend the existing image-only test to also exercise
prompt resolution: after calling ParseInput on the image-only JSON (the existing
variable imageOnly), call ResolvePrompt on the returned events (or the first
event) and assert the behavior you expect for image-only messages (e.g., that
ResolvePrompt returns a non-empty resolved prompt structure or an error-free
result and preserves the image in Events[0].Images). Update
TestValidateInputEventAllowsImageOnlyMessage (or add a new test like
TestResolvePromptAllowsImageOnlyMessage) to include the ParseInput ->
ResolvePrompt sequence and corresponding assertions referencing ParseInput and
ResolvePrompt to ensure the regression is covered.
🪄 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 Plus
Run ID: b43b8e79-d5ae-48e2-ac54-5d0969e0a078
📒 Files selected for processing (36)
internal/agent/loop.gointernal/agent/loop_images_test.gointernal/agent/types.gointernal/cli/app.gointernal/cli/exec.gointernal/cli/exec_images_gate_test.gointernal/cli/exec_images_test.gointernal/cli/exec_parse.gointernal/cli/exec_parse_image_test.gointernal/cli/exec_streamjson_images_test.gointernal/cli/exec_test.gointernal/imageinput/imageinput.gointernal/imageinput/imageinput_test.gointernal/modelregistry/vision.gointernal/modelregistry/vision_test.gointernal/providers/anthropic/image_test.gointernal/providers/anthropic/provider.gointernal/providers/gemini/images_test.gointernal/providers/gemini/provider.gointernal/providers/gemini/types.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/types.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.gointernal/tui/commands.gointernal/tui/commands_test.gointernal/tui/image_attach.gointernal/tui/image_attach_test.gointernal/tui/model.gointernal/tui/zeroline_view.gointernal/zeroline/render.gointernal/zeroruntime/helpers.gointernal/zeroruntime/image_types_test.gointernal/zeroruntime/seed_images_test.gointernal/zeroruntime/types.go
anandh8x
requested changes
Jun 8, 2026
anandh8x
left a comment
anandh8x
left a comment
Collaborator
There was a problem hiding this comment.
One blocking issue:
internal/streamjson/streamjson.go:190-200 accepts image-only message events earlier in validation (content == "" is allowed when len(images) > 0), but ResolvePrompt then drops empty content and errors when there are no text parts. runExec calls ResolvePrompt before ResolveImages (internal/cli/exec.go:505-509), so a valid image-only stream-json input never reaches the agent.
That contradicts the new protocol behavior in this PR: image-only message events are accepted by ParseInput, but zero exec --input-format stream-json rejects them with "must include at least one prompt or user message event." Please either make ResolvePrompt treat image-bearing user messages as a valid empty prompt, or otherwise handle the image-only case in resolveExecPrompt, and add an exec-level regression test for a stream-json message with content:"" and an image.
Everything else I checked looks solid: shared image loader, provider serialization, CLI/TUI vision gating, submit-time TUI re-check, and OpenAI user-role-only image parts. I also ran:
GOCACHE=/tmp/zero-go-cache go test ./...
Add a shared CloneImageBlocks helper in zeroruntime that deep-copies a slice of ImageBlock including each Data byte slice (returning nil for nil/empty input). Use it in SeedMessagesWithImages so a seeded user turn owns an independent copy of the caller's image bytes, and in the agent loop's copyMessages so image bytes are deep-copied alongside ToolCalls instead of staying aliased across history/request/result copies. Tests assert that mutating the caller's original Data bytes after seeding leaves the seeded message unchanged, and that copyMessages produces independent image bytes.
Resolve stream-json images before the prompt in the exec input path and tolerate an empty prompt when images are present, so an image-only message event (empty content + one image) proceeds with prompt="" and the image reaches the agent run instead of being rejected. A turn carrying neither text nor images still errors. In ResolveImages, reject an oversized payload from its ENCODED length (DecodedLen) BEFORE the base64 decode so a huge blob never allocates a decode buffer just to be capped afterward; the post-decode length check stays as a backstop. Tests cover the image-only exec turn reaching the agent and the pre-decode oversize rejection (asserting the size gate fires before any decode).
Replace os.ReadFile with os.Open + io.ReadAll over an io.LimitReader(f, MaxImageBytes+1). The os.Stat pre-check is only a fast-path hint (a non-regular file reports a misleading size and the file can grow between stat and read); the LimitReader is the real bound, so at most one byte past the cap is ever buffered and an oversized or unbounded source can never allocate a multi-gigabyte buffer. The file is closed via defer and the existing size message is reused.
cmdRegion emits TWO rows when ImageChips is set (a chip row above the input line), but the body/overlay height accounting assumed a one-row command region, overflowing the fixed frame by one line. Compute the fixed-row count from the actual command-region height (1 or 2 rows) so the composed frame stays exactly Height rows in both cases. A RenderChat test asserts the rendered frame line-count equals the requested height both with and without ImageChips set.
Route the inline --image= form through requiredInlineFlagValue, the same empty-rejection the other inline flags use, so --image= errors with "--image requires a value" instead of appending an empty image path. A parse test covers the empty inline value.
Collaborator
Author
|
Thanks — all 7 addressed in
@coderabbitai please re-review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
# Check CloneImageBlocks implementation
echo "=== zeroruntime/types.go ==="
cat internal/zeroruntime/types.go
echo ""
echo "=== zeroruntime/helpers.go ==="
cat internal/zeroruntime/helpers.go
echo ""
echo "=== internal/agent/loop.go (relevant section) ==="
rg -n "CloneImageBlocks|copyMessages|Images" internal/agent/loop.go | head -40
echo ""
echo "=== imageinput/imageinput.go ==="
cat internal/imageinput/imageinput.go
echo ""
echo "=== streamjson/streamjson.go (ResolveImages section) ==="
cat internal/streamjson/streamjson.go
echo ""
echo "=== zeroline/render.go ==="
cat internal/zeroline/render.go
echo ""
echo "=== cli/exec_parse.go (--image= handling) ==="
cat internal/cli/exec_parse.go
echo ""
echo "=== cli/exec.go (stream-json image-only branch) ==="
cat internal/cli/exec.goLength of output: 116325
No new issues found. The implementation is solid. [approve] 🐇 ✨ |
Collaborator
Author
|
@anandh8x good catch — this was exactly right, and it's now fixed (it overlapped with a CodeRabbit finding so it landed in the same batch, I took your second suggested approach — handle the image-only case in streamImages, err := streamjson.ResolveImages(events) // resolve images first
if err != nil { return "", nil, execUsageError{err.Error()} }
prompt, perr := streamjson.ResolvePrompt(events)
if perr != nil {
// An image-only message event (content:"" + >=1 image) is valid: ResolvePrompt
// rejects empty content, but with images present we proceed with an empty prompt.
if len(streamImages) == 0 {
return "", nil, execUsageError{perr.Error()}
}
}
return prompt, streamImages, nilSo an image-only Added the exec-level regression test you asked for — Please re-review when you get a chance. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/imageinput/imageinput.go (1)
53-53: 💤 Low valueConsider checking the Close error for completeness.
While ignoring
defer file.Close()errors is idiomatic for read-only local files, some filesystems can surface read errors at close time. This is a very minor point given the read error is already checked at line 55.Optional: check Close error
- defer file.Close() + defer func() { _ = file.Close() }()Or explicitly handle it if you prefer stricter error checking (though in this context it's not necessary).
🤖 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 `@internal/imageinput/imageinput.go` at line 53, Replace the simple defer file.Close() with a deferred closure that checks and surfaces Close errors: defer func() { if cerr := file.Close(); cerr != nil { if err == nil { err = fmt.Errorf("closing file: %w", cerr) } else { /* optionally log: fmt.Errorf("closing file (suppressed): %v", cerr) */ } } }() so any filesystem error on Close is returned (or at least logged); reference the existing file.Close() defer and the function's named return error variable (err) when locating where to change this in internal/imageinput/imageinput.go.
🤖 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.
Nitpick comments:
In `@internal/imageinput/imageinput.go`:
- Line 53: Replace the simple defer file.Close() with a deferred closure that
checks and surfaces Close errors: defer func() { if cerr := file.Close(); cerr
!= nil { if err == nil { err = fmt.Errorf("closing file: %w", cerr) } else { /*
optionally log: fmt.Errorf("closing file (suppressed): %v", cerr) */ } } }() so
any filesystem error on Close is returned (or at least logged); reference the
existing file.Close() defer and the function's named return error variable (err)
when locating where to change this in internal/imageinput/imageinput.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d222535-8f67-4f74-b4dc-4ff467e2e2aa
📒 Files selected for processing (13)
internal/agent/loop.gointernal/agent/loop_images_test.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_parse_image_test.gointernal/cli/exec_streamjson_images_test.gointernal/imageinput/imageinput.gointernal/streamjson/streamjson.gointernal/streamjson/streamjson_test.gointernal/zeroline/render.gointernal/zeroline/render_test.gointernal/zeroruntime/helpers.gointernal/zeroruntime/seed_images_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/zeroline/render_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cli/exec_parse.go
- internal/agent/loop.go
- internal/streamjson/streamjson_test.go
- internal/streamjson/streamjson.go
- internal/cli/exec.go
anandh8x
requested changes
Jun 8, 2026
anandh8x
left a comment
anandh8x
left a comment
Collaborator
There was a problem hiding this comment.
One remaining blocking issue:
internal/imageinput/imageinput.go:36-55 still accepts non-regular filesystem entries. The new bounded read protects memory once reading starts, but os.Open(resolved) can block before the LimitReader is reached for a FIFO/named pipe with no writer. That means zero exec --image <fifo> or TUI /image <fifo> can hang the process/UI indefinitely.
Please reject non-regular files immediately after os.Stat, e.g. if !info.Mode().IsRegular() { return ..., fmt.Errorf("image file must be a regular file: %s", path) }, and add a small loader test for a directory or FIFO/non-regular path. Symlinks to regular files can still work because os.Stat follows them.
The previous image-only stream-json blocker is fixed: images are resolved before prompt text, image-only turns proceed with an empty prompt, and there is exec-level coverage for that path.
Tests run:
GOCACHE=/tmp/zero-go-cache go test ./...
Collaborator
Author
|
@anandh8x fixed in if !info.Mode().IsRegular() {
return zeroruntime.ImageBlock{}, fmt.Errorf("image file must be a regular file: %s", path)
}This runs before |
anandh8x
approved these changes
Jun 8, 2026
anandh8x
left a comment
anandh8x
left a comment
Collaborator
There was a problem hiding this comment.
Re-review after f626b50.
The previous blockers are fixed:
- image-only stream-json turns now resolve images before prompt text and proceed with an empty prompt when images are present.
imageinput.LoadFilenow rejects non-regular files before opening, so directories/FIFOs/devices cannot hang the CLI/TUI path before the bounded read.
I rechecked the loader, stream-json, CLI image flow, deep-copying, and provider serialization paths. No further blocking issues found.
Tests:
GOCACHE=/tmp/zero-go-cache go test ./internal/imageinput ./internal/cli ./internal/streamjson ./internal/zeroruntime ./internal/agent ./internal/zeroline ./internal/providers/openai ./internal/providers/anthropic ./internal/providers/geminiGOCACHE=/tmp/zero-go-cache go test ./...git diff --check origin/main...HEAD
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/imageinput/imageinput.go (1)
51-55: 💤 Low valueUpdate comment to reflect that non-regular files are already rejected.
The comment mentions "a non-regular file reports a misleading size," but line 44 now rejects non-regular files before reaching this code. The bounded-read rationale should focus on stat/read races for regular files that grow between stat and read.
📝 Suggested comment update
- // Bounded read: the os.Stat above is only a fast-path hint (a non-regular - // file reports a misleading size, and the file can grow between Stat and the - // read). A LimitReader of MaxImageBytes+1 is the real bound — at most one byte - // past the cap is ever buffered, so an oversized or unbounded source (e.g. a - // FIFO) can never allocate a multi-gigabyte buffer just to be discarded. + // Bounded read: the os.Stat size is only a fast-path hint (the file can grow + // between Stat and the read). A LimitReader of MaxImageBytes+1 is the real + // bound — at most one byte past the cap is ever buffered, so an oversized + // source can never allocate a multi-gigabyte buffer just to be discarded.🤖 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 `@internal/imageinput/imageinput.go` around lines 51 - 55, Update the comment above the bounded-read logic to remove the outdated note about "a non-regular file reports a misleading size" (non-regular files are already rejected earlier) and instead explain that the LimitReader(MaxImageBytes+1) protects against races where a regular file can grow between os.Stat and the subsequent read; mention the intent that at most one byte past the cap is buffered and this prevents allocating large buffers for oversized/unbounded sources. Reference MaxImageBytes, LimitReader and os.Stat in the updated comment.internal/imageinput/imageinput_test.go (1)
87-102: 💤 Low valueConsider adding a test for symlinks to regular files.
The production code comment (imageinput.go:41-42) states that symlinks to regular files should pass. A test verifying this positive case would strengthen coverage and document the intended behavior.
🧪 Suggested test addition
func TestLoadFileSymlinkToRegularFile(t *testing.T) { root := t.TempDir() png := []byte{ 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x06, 0x00, 0x00, 0x00, 0x1F, 0x15, 0xC4, 0x89, 0x00, 0x00, 0x00, 0x0A, 0x49, 0x44, 0x41, 0x54, 0x78, 0x9C, 0x63, 0x00, 0x01, 0x00, 0x00, 0x05, 0x00, 0x01, 0x0D, 0x0A, 0x2D, 0xB4, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4E, 0x44, 0xAE, 0x42, 0x60, 0x82, } target := filepath.Join(root, "pic.png") if err := os.WriteFile(target, png, 0o644); err != nil { t.Fatalf("write png: %v", err) } symlink := filepath.Join(root, "link.png") if err := os.Symlink(target, symlink); err != nil { t.Skipf("symlink not supported: %v", err) } // Symlink to regular file should succeed (Stat follows symlinks). block, err := LoadFile("link.png", root) if err != nil { t.Fatalf("LoadFile symlink: %v", err) } if block.MediaType != "image/png" { t.Errorf("MediaType = %q, want image/png", block.MediaType) } }🤖 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 `@internal/imageinput/imageinput_test.go` around lines 87 - 102, Add a new unit test named TestLoadFileSymlinkToRegularFile to internal/imageinput/imageinput_test.go that creates a small valid PNG file in t.TempDir(), writes it as e.g. "pic.png", creates a symlink "link.png" pointing to that target (skip the test if os.Symlink is unsupported), then call LoadFile("link.png", root) and assert no error and that the returned block.MediaType == "image/png"; this verifies that LoadFile follows symlinks to regular files as documented. Ensure the test uses the same LoadFile function and temp directory setup pattern as existing tests and includes the small PNG byte slice and permission 0o644 when writing the target file.
🤖 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.
Nitpick comments:
In `@internal/imageinput/imageinput_test.go`:
- Around line 87-102: Add a new unit test named TestLoadFileSymlinkToRegularFile
to internal/imageinput/imageinput_test.go that creates a small valid PNG file in
t.TempDir(), writes it as e.g. "pic.png", creates a symlink "link.png" pointing
to that target (skip the test if os.Symlink is unsupported), then call
LoadFile("link.png", root) and assert no error and that the returned
block.MediaType == "image/png"; this verifies that LoadFile follows symlinks to
regular files as documented. Ensure the test uses the same LoadFile function and
temp directory setup pattern as existing tests and includes the small PNG byte
slice and permission 0o644 when writing the target file.
In `@internal/imageinput/imageinput.go`:
- Around line 51-55: Update the comment above the bounded-read logic to remove
the outdated note about "a non-regular file reports a misleading size"
(non-regular files are already rejected earlier) and instead explain that the
LimitReader(MaxImageBytes+1) protects against races where a regular file can
grow between os.Stat and the subsequent read; mention the intent that at most
one byte past the cap is buffered and this prevents allocating large buffers for
oversized/unbounded sources. Reference MaxImageBytes, LimitReader and os.Stat in
the updated comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 76bdeaa1-2d6c-4114-b181-ac31caf72c59
📒 Files selected for processing (2)
internal/imageinput/imageinput.gointernal/imageinput/imageinput_test.go
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
3 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
Adds image (multimodal) input end-to-end: a user can attach local image files to a chat turn so a vision-capable model can see them, across all three providers and both input surfaces. Strictly additive — every text-only path is byte-for-byte unchanged. No new module dependencies.
internal/zeroruntime): optionalMessage.Images []ImageBlock(ImageBlock{MediaType string; Data []byte}— raw bytes), aNormalizeImageMediaTypeallow-list helper (png/jpeg[jpg→jpeg]/gif/webp), andSeedMessagesWithImages(SeedMessagesdelegates with nil — signature unchanged).image/source.base64content blocks, GeminiinlineDataparts, OpenAIimage_urldata-URI parts (this requiredchatMessage.Contentstring→any; an empty-contentomitemptyguard keeps text-only requests byte-identical). Image-only turns still emit; only the user role carries images.internal/modelregistry):SupportsVision; when the effective model lacks vision, images are dropped with a warning (never errors the run). Unknown/custom model ids fail closed (drop).internal/cli): repeatablezero exec --image <path>— reads, sniffs MIME, normalizes, caps at 10 MiB, base64-encodes; threaded into the agent run through the vision gate.internal/streamjson): animagesfield onmessageinput events (whitelisted only on message events; allows empty content when images present), resolved end-to-end into the agent run.internal/tui): a/image <path>command with removable[img: name]chips above the input (both skins),/image clear, vision-refusal at attach and re-checked at submit; images thread into the turn and clear after.internal/imageinput): oneLoadFile(read/sniff/normalize/cap,os.Statpre-check) used by both the CLI and TUI — no duplicated logic.Image sources are local files only (no remote URLs / clipboard). Allowed types: png, jpeg, gif, webp.
Test Plan
go build ./.../go vet ./...cleango test ./...green (new tests acrosszeroruntime, all 3 providers,modelregistry,imageinput,cli,streamjson,tui)go test -race ./internal/{cli,tui,imageinput,providers,streamjson,agent}/...greenGOOS=windows GOARCH=amd64 go build ./...greengit diff --checkcleanReviewer focus
Content string→anychange keeps an empty-content message'scontentkey omitted (regression-guarded; mutation-tested).--imagefile and a stream-jsonimagesevent both reachagent.Options.Images→ the provider wire format (covered byTestRunExecStreamJSONImageReachesAgent).Summary by CodeRabbit
New Features
Behavior
Tests
Documentation