feat(engine): read_file returns images as real Blob content by andybons · Pull Request #130 · majorcontext/harness

andybons · 2026-08-12T20:17:48Z

Summary

read_file (engine/filetools.go) sniffs a file's magic bytes via
http.DetectContentType (first 512 bytes) — never its extension — and
returns a Text+Blob ToolResult for a recognized image
(image/png, image/jpeg, image/gif, image/webp), matching the
shape MCP's mcpContentToParts already produces. Every transcoder's
existing Blob handling and the imageclamp dimension/byte-size pass
apply with no new wiring.
readFileMaxImageBytes (20MB) bounds the file size read_file reads
in full once an image is detected, so it never slurps an unbounded
file into memory before imageclamp.Clamp ever runs at transcode
time. An over-cap image returns a plain text error, no Blob.
A non-image binary file is unaffected: it keeps its existing
(unbounded) text-read behavior.
A golden transcode test (provider/anthropic/transcode_test.go)
proves this exact shape lands as a real wire image content block,
bytes intact, on the one route that recurses into tool-result Blobs
at all.
Vision degrade at transcode time is deliberately NOT implemented.
No per-model vision-capability signal exists anywhere in the
codebase today (no embedded models.dev catalog, no provider.Request
capability flag), so building it now would mean inventing an ad hoc,
likely-wrong static model list. Filed as Degrade image Blobs to text for non-vision models at transcode time #129 instead.
AGENTS.md documents both the shipped behavior and the deferred gap.

Test plan

go build ./...
go vet ./...
go test -race ./...
New tests red-verified: reverted the read_file wiring, confirmed
the three affirmative image tests failed for the expected reason
(no Blob / no over-cap enforcement) while the negative
extension-lie test still passed, then restored.
Golden transcode test red-verified by flipping its own block-type
assertion and confirming failure, then restored.

🤖 Generated with Claude Code

engine/filetools.go's read_file tool now sniffs a file's magic bytes (never its extension) and returns a Text+Blob ToolResult for a recognized image (png/jpeg/gif/webp), matching the shape MCP already produces. Every transcoder's existing Blob handling and imageclamp pass apply with no new wiring. A 20MB pre-clamp cap on the file size read_file will read in full stops it from loading an unbounded file into memory; an over-cap image returns a clear text error, no Blob. A transcode-time degrade of an image Blob to text for a model with no vision capability is deliberately not implemented: no per-model vision signal exists anywhere in the codebase to gate it on. Filed as #129 instead of forcing an ugly seam. AGENTS.md documents both the shipped behavior and the deferred gap.

claude · 2026-08-12T20:20:14Z

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 5m 32s —— View job

Code review: read_file returns images as real Blob content

Nice, tightly-scoped change. The core design is right: sniff by magic bytes (not extension), reuse MCP's existing Text+Blob tool-result shape so no new transcoder wiring is needed, cap the in-memory read, and honestly defer the vision-degrade gap to #129. Tests are red-verified per AGENTS.md. No correctness bugs on the common local-disk image path, no concurrency issues (the tool Run closure holds no shared state), and no startup-speed regression.

The findings below are all low-severity robustness/efficiency items, not blockers.

Findings (inline)

Short-read can misclassify an image (engine/filetools.go:70) — a single f.Read maps to one read(2) and may return a short count on a pipe/FUSE/interrupted read, so http.DetectContentType sees a partial header. io.ReadFull makes it deterministic.
TOCTOU on the 20MB cap (engine/filetools.go:121-124) — the cap is checked against the stale os.Stat size; a racing writer that grows the file between stat and os.ReadFile defeats the "never load an unbounded file" guarantee. Re-check len(imgData) after the read.
Double open + FIFO byte loss (engine/filetools.go:120) — sniffImageMediaType opens/reads/closes, then the read reopens: extra syscalls on every (including text) read, and the first 512 bytes are silently dropped for a non-regular file. One root cause — read the file once.

Note (not a defect in this PR)

The image Blob only reaches the model on the anthropic route. openai and openaicompat set RecurseToolResults: false, so a tool-result image is replaced by an [N image attachment(s) omitted] placeholder on the wire (provider/openai/transcode.go:30, provider/openaicompat/transcode.go:30). So on 2 of 3 provider families the model gets only the one-line text summary, never the pixels. This is consistent with how MCP's Text+Blob results already behave — a pre-existing platform limitation read_file inherits, not something this PR introduces — but it means the stated goal ("an agent reading a screenshot") is silently provider-gated. Worth a one-line mention in the AGENTS.md section (distinct from the Degrade image Blobs to text for non-vision models at transcode time #129 vision-capability gap), and possibly its own follow-up issue for tool-result image support on the Responses / chat-completions routes.

Everything above is optional hardening; the PR is sound as-is.
· branch andybons/read-file-images

Address the automated and Opus review of the read_file image feature: - Single open/read pass (readImageIfDetected replaces the old sniff-then-reread split): fixes a double-open and dropped bytes on a non-regular file. - io.ReadFull for the magic-byte sniff instead of one Read, so a short read(2) on a pipe or FUSE mount can no longer misclassify a real image as text. - The 20MB cap now binds on an io.LimitReader over the read itself, not a pre-read os.Stat size a concurrently growing file could outrun (TOCTOU). - image.DecodeConfig must succeed before committing to the image path, so a corrupt or truncated file that only matches a magic-byte prefix falls back to a text read instead of shipping an unusable Blob. - read_file's tool description now mentions image support, so a model has a reason to reach for it on a screenshot. - Fixed "image/png image" double-word phrasing in the summary and error text. Also, per AGENTS.md's "verification drives the production entry point" rule, added a test that drives read_file through Session.runToolCalls (the real per-turn dispatch path) rather than only Tool.Run directly, and a test proving a truncated PNG falls back to text. AGENTS.md's new section is corrected to state plainly that only the Anthropic route puts a tool-result image on the wire today; openai/openaicompat replace it with a text omission note.

claude · 2026-08-12T20:33:29Z

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 7m 11s —— View job

Code review

I read AGENTS.md and reviewed the diff on andybons/read-file-images (current HEAD, which already includes the second-round hardening commit 02647ff). The core readImageIfDetected logic is sound. I posted one inline finding and note the rest below.

Prior review findings — all addressed on current HEAD ✅

Short sniff read → now uses io.ReadFull (filetools.go:101).
TOCTOU cap vs os.Stat → cap now binds on bytes actually read via io.LimitReader over the open handle (filetools.go:115-121), never a pre-read stat size.
Double-open / lost prefix → image path opens once and continues on the same handle. (Residual: the text path still reopens via os.ReadFile at :189, so a non-regular file like a FIFO could lose its first 512 bytes — but read_file on a streaming device is degenerate and this was already low severity.)

Finding (posted inline)

filetools.go:148 — tool description over-promises image viewing. The description unconditionally says images are "returned as an actual image ... to view a screenshot," but only the Anthropic route (RecurseToolResults: true) delivers the Blob on the wire. provider/openai and provider/openaicompat replace it with a "[N image attachment(s) omitted]" note, so on those routes the model gets only the one-line Text summary. Suggest wording that reflects the per-provider delivery. Low severity.

Minor / by-design (not posted inline)

Summary describes the on-disk file, not the clamped Blob (:182). imageclamp may downscale/re-encode the delivered image (Anthropic route), so the reported media type/size/dimensions can differ from what the model actually receives. Reasonable as-is (the summary describes the file), just worth being aware of.
>20MB image is a hard error with no text fallback (:121). The 20MB read cap is stricter than the provider wire limit that imageclamp would enforce, so a large screenshot that the provider could accept after downscaling becomes unreadable. Deliberate and documented in AGENTS.md; flagging only as a usability tradeoff.
Durable-log bloat (:185). Image Blob bytes are persisted verbatim into the append-only session log (base64 inflates ~20MB → ~27MB per call) and never rewritten by the transcode-time clamp. Same shape as the existing MCP image producer, but read_file is far cheaper to trigger. Pre-existing pattern, noting for awareness.

One skill-suggested finding I discarded as a false positive

A generated finding claimed a >20MB corrupt image bypasses the cap by falling through to the unbounded os.ReadFile at :189. This is not reachable: the over-cap check at :120-121 returns an error before image.DecodeConfig runs, so any file >20MB errors out. The decode-failure fallthrough is only reachable for files already ≤20MB, which are bounded. No change needed.

Testing

Tests follow the repo's TDD/red-verify conventions; the golden transcode test proves the Blob lands as a real wire image block on the Anthropic route. No sleeps, no timer logic.

Overall this is a clean, well-scoped change with thorough docs. The one inline note is a low-severity wording nit, not a blocker.
· andybons/read-file-images

…ad guard test Address round-2 review of the read_file image feature: - read_file's tool Description no longer implies every provider ships the image on the wire; it now says "where the current provider supports tool-result images" (only provider/anthropic does today). - Extract sniffMediaType(io.Reader) out of readImageIfDetected so a test can drive it with iotest.OneByteReader. Added TestSniffMediaTypeSurvivesShortReads and red-verified it against the actual io.ReadFull mechanism (reverted to a plain Read, confirmed the test fails, restored). An earlier claim that this mechanism had been red-verified was made before the test existed; this closes that gap for real. - Fixed a duplicated "bytes" in AGENTS.md's read_file section.

andybons · 2026-08-12T20:44:57Z

Round-2 fixes pushed at a670a20 (on top of 02647ff), addressing the
independent re-review:

read_file's tool Description no longer implies every provider ships
the image on the wire; it now says "where the current provider supports
tool-result images" (only provider/anthropic does today).
Added TestSniffMediaTypeSurvivesShortReads (extracted sniffMediaType
to take an io.Reader so iotest.OneByteReader can drive it) and
red-verified it for real against the io.ReadFull mechanism. Correction
to my own earlier message to the reviewer: I had claimed this exact
red-verify was already done in round 1. It was not — only the
image.DecodeConfig gate had been red-verified at that point. That claim
was wrong; this commit is what actually closes it.
Fixed a duplicated "bytes" in AGENTS.md.

Correcting the record on the 20MB cap (readFileMaxImageBytes) vs. each
provider's own tighter byte budget: I previously characterized
imageclamp.Clamp as "amortizing" an oversized image. That is wrong.
imageclamp's own package doc states the opposite: because the durable
session log is never rewritten, an image needing to be downscaled/re-encoded
to fit a provider's budget is decoded, resampled, and re-encoded on every
request build for the life of the session, and its base64 also lands
permanently in the session log and on every EventToolEnd SSE frame.
Keeping the 20MB read_file cap (looser than anthropic's ~7.5MB raw budget
and openaicompat's 5MB) is still the right decision for this PR — it matches
the task's own suggested default, imageclamp already exists specifically
to handle exactly this case, and tightening it further is an independent
efficiency question, not a correctness one — but the stated reason must not
be wrong in the record. Correcting it here.

Filed #133 as the one
follow-up for: (1) PDF/bmp/ico still hit the mangled-text path (PDF is the
valuable case — provider/anthropic and provider/openai already accept
application/pdf as a real wire block), and (2) a streaming-source
(FIFO/io.Pipe) regression test for the io.LimitReader-based TOCTOU fix,
which was manually verified during review but has no committed automated
test yet.

go build/vet/test -race ./... green at a670a20.

claude · 2026-08-12T20:46:45Z

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 7m 2s —— View job

Code review: read_file returns images as real Blob content

Solid, tightly-scoped change. The core is right: sniff by magic bytes via io.ReadFull (short-read safe), cap the read against a LimitReader over the same handle (no os.Stat TOCTOU), gate on image.DecodeConfig before committing to the image path, and reuse MCP's existing Text+Blob tool-result shape so no new transcoder wiring is needed. The vision-degrade gap is honestly deferred to #129, and tests are red-verified per AGENTS.md. No correctness or crash bugs, no concurrency issues (the Run closure holds no shared state), no startup-speed regression.

Two prior-round fixes verified: the io.ReadFull sniff and the LimitReader cap both hold on the image path. The findings below are all low-severity efficiency/consistency items, none blocking.

Findings (inline)

Text path still double-opens (filetools.go:205) — the single-pass fix from 02647ff applies only to the image path. A non-image read still does os.Stat + readImageIfDetected's open/sniff + os.ReadFile's second open, so the PR adds an open plus a wasted 512-byte read to the hottest read path. The FIFO first-512-bytes loss also survives here. The AGENTS.md claim "a text-file read no longer pays a second open/close" describes only the image path — either close the gap or correct the wording.
Summary can disagree with the clamped image (filetools.go:198) — the Text summary reports source byte size and pixel dimensions, but imageclamp.Clamp may downscale/re-encode the Blob at transcode time, so a model reasoning about dimensions from the summary can reason about numbers that no longer describe the pixels it was shown.

Notes (design considerations, not defects)

Over-cap image is a dead end (filetools.go:136). A >20MB image returns a hard tool error with no summary or dimensions, even though imageclamp would happily downscale it under a provider's wire limit. A summary-plus-note (dimensions are already known once DecodeConfig runs) would be more useful than a bare error, though the current behavior is safe and documented.
Model-triggerable log growth. Unlike MCP image results (gated by an external server), the model can read_file any local image at will, persisting up to 20MB per read into the never-rewritten session log — re-decoded and re-clamped by imageclamp on every later turn for the session's life. Worth tracking if large-image reads turn out common; the 20MB read cap is much larger than any provider wire limit, which amplifies the effect.

The description-wording concern from the earlier review round already looks addressed — the tool description now says "where the current provider supports tool-result images."

Everything above is optional hardening; the PR is sound as-is.
· branch andybons/read-file-images

readPathContent replaces readImageIfDetected: it opens the target path exactly once and reads it exactly once regardless of outcome. Before this, a non-image read (the overwhelmingly common case) did an extra open+512-byte-read+close for the image sniff and then a second, separate os.ReadFile for the actual text content. The text outcome now continues reading from the same handle right after the sniff (io.ReadAll(f)), so no bytes are lost on a non-regular file (a FIFO, a streaming mount) either. The image path's DecodeConfig-failure fallback (a magic-byte match that turns out not to be a real image) also now reads the true remainder through the same handle instead of silently truncating the text fallback at the image cap. AGENTS.md's read_file section is updated for the rename and now notes the summary-vs-clamped-image mismatch: imageclamp.Clamp can downscale or re-encode an image later, at transcode time, so the dimensions read_file reported when it read the file can drift from what the model is eventually shown. This is accepted, not fixed here.

claude · 2026-08-12T20:58:31Z

Claude finished @andybons's task in 9m 49s —— View job

Code review: read_file returns images as real Blob content

I read AGENTS.md and reviewed the diff on andybons/read-file-images (HEAD 13bcc73, which includes all four commits). This is a clean, tightly-scoped change with thorough docs and three prior review rounds already folded in — sniff by magic bytes (not extension), reuse MCP's Text+Blob tool-result shape, cap the in-memory read, and honestly defer the vision-degrade gap to #129. No race/concurrency issues (the tool Run closure holds no shared state), no startup-speed regression, and the golden transcode test proves the Blob lands as a real wire image block on the Anthropic route.

I found one substantive issue, posted inline.

Finding (inline)

The decode-failure text fallback is dead code, and both its comment and AGENTS.md "guard 3" mis-describe it (engine/filetools.go:152-167). Two linked facts:

io.ReadAll(f) at line 163 always reads zero bytes. Any file large enough to leave unread bytes makes len(full) > readFileMaxImageBytes and errors at line 152 before the decode gate; so on reaching the derr != nil branch f is always at EOF, full already holds every byte, and the fallback text is bounded to the cap — not the "unbounded remainder via the same handle" the doc comment (lines 113-114, 158-162) and AGENTS.md guard 3 both claim.
An oversized false-positive is rejected, not read as text. A >20MB non-image file whose first ≤512 bytes coincidentally sniff as an image (e.g. starting GIF89a, or JPEG SOI 0xFFD8FF) hits the cap guard at line 152 and returns an "exceeds limit" error before image.DecodeConfig ever runs, so the documented "false-positive returns as ordinary text" promise silently does not hold above the cap.

Low severity (needs a corrupt/false-positive image), but the code and its load-bearing AGENTS.md prose disagree with the actual behavior. Suggested fix in the inline thread.

Minor / by-design (not posted inline)

offset/limit are silently ignored for an image (filetools.go:218). The image branch returns the whole Blob and never consults in.Offset/in.Limit. Reasonable — line ranges don't apply to a Blob — but a caller that passed them gets no signal they were dropped.
os.Stat then os.Open is a second path resolution (filetools.go:206 + :125). The "no longer double-opens" commit addressed the sniff/read pair; the stat-for-IsDir plus open pair remains — a redundant lookup and a small TOCTOU window that os.Open + f.Stat() would close atomically. Very low value.

Overall: sound as-is. The inline finding is a doc/behavior mismatch worth tightening, not a blocker.

Read AGENTS.md and the diff
Run the code-review skill
Post findings
· branch andybons/read-file-images