fix(media): drop oldest images on over-count instead of refusing #8
Reference in New Issue
Block a user
Delete Branch "fix/image-overflow-drop-oldest"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem (observed live)
media.Normalizerefused (ErrUnsupported) when a request carried more images than the target'sMaxImagesPerReq, on the theory that a failover chain would try a roomier target. In practice the chain's targets share the same cap. An agent loop that accumulates a preview image per iteration (mort's scaddywrite_scad) blows past the cap, every target rejects —request carries 9 images, target allows at most 8 per request(ollama-cloud, cap 8) — and the whole run dies (all chain targets failed).Fix
Over-count no longer fails.
Normalizekeeps the most-recentMaxImagesPerReqimages and replaces each older one with a short text placeholder ([earlier image omitted to fit this model's per-request image limit]), preserving each message's turn structure and signalling the model that an image was elided. The most-recent images are the relevant ones in an iterative run.MaxImagesPerReqstays the per-model knob (0 still = no image support).SupportsImages/ MIME / byte-budget / dimension normalization unchanged; the provider-side count check remains as a backstop.Test
TestNormalizeTooManyImages_DropsOldest: 3 images across 2 messages, cap 2 → 2 kept (the most recent), 1 placeholder, no error, oldest dropped, input request unmutated. Fullmedia+ majordomo suites green (-race).Note
The previous "refuse + let the chain try a roomier target" behavior was deliberate (honesty over silently changing the question). This trades that for keeping iterative agent runs alive, mitigated by (a) keeping the most-recent images and (b) the explicit placeholder so the model knows. Happy to gate it behind an opt-in policy instead if you'd prefer the old default preserved.
🤖 Generated with Claude Code
media.Normalize refused (ErrUnsupported) when a request carried more images than the target's MaxImagesPerReq, on the theory that a failover chain would try a roomier target. In practice the chain's targets share the same cap — an agent loop that accumulates a preview image per iteration (e.g. scaddy's write_scad) blows past the cap, EVERY target rejects ("9 images, target allows at most 8"), and the run dies. Observed live on ollama-cloud (cap 8). Now: over-count keeps the most-recent MaxImagesPerReq images and replaces each older one with a short text placeholder ("[earlier image omitted to fit this model's per-request image limit]"), preserving each message's turn structure and telling the model an image was elided. The most-recent images are the relevant ones in an iterative run. Copy-on-write; the input request is never mutated. The per-model threshold stays configurable via Capabilities.MaxImagesPerReq (0 still means no image support); SupportsImages / MIME / byte-budget / dimension behavior is unchanged, and the provider-side count backstop remains. Test: TestNormalizeTooManyImages_DropsOldest — 3 images, cap 2 → 2 kept (the most recent), 1 placeholder, no error, oldest dropped, input unmutated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>🪰 Gadfly review —
glm-5.2:cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
media/media.go:46-152: the only new data introduced into the request is the constantimageOverflowPlaceholder(line 110), a fixed string with no user-controlled/tainted content — no prompt-injection, template, SQL, command, or path vector. Dropped images are selected purely by count/order, not by any attacker-influenced key.normalizeImage(MIME sniff, byte budget, dimension caps) at lines 83-101, so the overflow path does not bypass any existing input validation on the retained image bytes.MaxImagesPerReq == 0still means "no image support" and is gated bySupportsImages()at line 58 (llm/capabilities.go:37), so the over-count drop path is only reachable for targets that explicitly support images — no text-only target silently accepts images.🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the correctness of the change through the following checks:
Drop math is exact.
n = total - caps.MaxImagesPerReqwhentotal > cap.dropOldestImagesreplaces the firstnimage parts front-to-back, leaving exactlytotal - n = capimages. Sincen < totalalways holds (becausetotal > cap), the loop never runs short of images to drop. Confirmed by readingmedia/media.go:70-71,116-143.Oldest-first ordering is correct. Messages are iterated in slice order (oldest→newest in conversation history) and image parts within a message are iterated in order, so the oldest images are dropped and the most-recent
MaxImagesPerReqsurvive.media/media.go:124-141.MaxImagesPerReq == 0is safe. The overflow branch is only reachable aftercaps.SupportsImages()passes (media/media.go:58), andSupportsImages()returnsMaxImagesPerReq > 0(llm/capabilities.go:37). Sototal - caps.MaxImagesPerReqis never evaluated with a zero cap in the overflow path; no negative-nor refuse-when-unsupported regression.Copy-on-write is sound.
dropOldestImagesallocates a newMessagesslice and only allocates newPartsfor messages whose images are actually replaced; messages with no images (or past the drop point) share theirPartsslice read-only. The subsequentnormalizeImageloop (media/media.go:74-103) operates on the result ofdropOldestImagesand does its own copy-on-write for transforms. The input request is never mutated. Verified atmedia/media.go:116-143.Provider-side backstop stays consistent. After dropping, the remaining image count equals
cap, so the provider checks (images > caps.MaxImagesPerReq) inprovider/anthropic/anthropic.go:219andprovider/openai/model.go:214evaluate tocap > cap= false and pass. No double-rejection.Partial-drop within a multi-image message is handled. When a message has several images and the drop count is exhausted mid-message, only the earlier images in that message become placeholders and the rest are kept, which is consistent with "keep most-recent."
media/media.go:132-139.No correctness bugs found.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
media/media.go:61-69— The 9-line inline comment restates the package-level doc atmedia/media.go:12-17almost verbatim (same "agent loop accumulating a preview image per iteration" rationale, same "0 = no image support" note). Two copies of the same rationale must now be kept in sync; if one drifts the other misleads. Trim the inline comment to 2-3 lines and let the package doc own the full justification.media/media.go:116-143vs74-103—dropOldestImagesperforms its own full copy-on-write (out.Messages = make(...); copy(...)at lines 121-122, plus per-messagepartsrebuild), and the mainNormalizeloop has a second, independent COW mechanism (copiedMessages/copiedPartsflags at lines 75, 90-99). In the overflow path both fire: afterreq = dropOldestImages(...)(line 71), the first image transform re-allocates and re-copiesout.Messagesat lines 91-92 becausecopiedMessagesis still false. The Messages slice is thus allocated and copied twice. The two COW idioms do the same job redundantly; consider havingdropOldestImagesset upout/copiedMessagesstate so the main loop skips its re-copy, or unify the strategy.media/media.go:131-140— Thepartsslice is rebuilt (make([]llm.Part, 0, len(...))) for every image-bearing message entered whiledropped < n, including the boundary message wheredroppedreachesnpartway through and the remaining parts are appended unchanged. For that message the allocation is unnecessary, and it reads as "always rebuild" rather than "rebuild only if something changed," inconsistent with the main loop'schanged-gated copy. Low impact.No dead code or naming problems found;
hasImagePartandimageOverflowPlaceholderare reasonably named and scoped.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, I verified the full
Normalizeflow (media/media.go:46-105) anddropOldestImages(media/media.go:116-143):dropOldestImagesreplaces overflow images with cheap text placeholders before the main loop, sonormalizeImage(the expensive decode/scale/encode path) is only ever called on the survivingMaxImagesPerReqimages. With a large overflow (e.g. 1000 images, cap 8), only 8 images are decoded — this is actually better than the old behavior, which would have errored and killed the run anyway.dropOldestImagesis only called under the sametotal > caps.MaxImagesPerReqcondition that previously returned an error. The fast paths (no images, images-already-fit) are unchanged.dropOldestImagesbreaks early oncedropped >= n. No I/O or blocking calls introduced.One negligible observation (not a material issue): on the overflow path,
dropOldestImagesallocates a freshMessagesslice and per-messagePartsslices, and then the main normalize loop at lines 90-99 alloc🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling & edge-cases lens, I verified the following by reading the code:
dropOldestImagesn-guard (media/media.go:117):n <= 0returnsrequnchanged — safe. The only caller passestotal - caps.MaxImagesPerReq, and since the call site is gated bytotal > caps.MaxImagesPerReqwithSupportsImages()(which requiresMaxImagesPerReq > 0,llm/capabilities.go:37) already checked,nis always strictly positive here. No negative/zero overflow path.Copy-on-write correctness (
media/media.go:120-141): A freshMessagesslice is allocated and the message structs are copied. NewPartsslices are allocated only for messages containing image parts; messages without images keep their original (shared, unmutated)Parts. The inner loop only mutates the newly allocatedpartsslice, never the input. I confirmedllm.Message/llm.Partare plain value types (llm/content.go), so the shallow copy is sufficient — no aliasing of mutable state. The test's unmutation assertion (media_test.go) corroborates this.Under-drop risk vs. provider backstop:
totalinNormalizeand the drop count indropOldestImagesboth use the samep.(llm.ImagePart)type assertion over the same message order, so exactlynimages are dropped, leaving exactlyMaxImagesPerReq. I checked the provider backstops (provider/anthropic/anthropic.go:219,provider/google/google.go:179,provider/ollama/wire.go:118) — all reject onlycount > MaxImagesPerReq(strictly greater), so an equal-count request passes. No mismatch that would cause the backstop to re-reject whatNormalizejust trimmed.Nil/empty edge cases:
req.Messagesempty →total == 0→ early return atmedia.go:55. A message with nilParts→hasImagePartreturns false → skipped, no panic on range-over-nil.No swallowed errors: The change removes an error return and replaces it with a non-erroring transform; this is the intended behavior change, not an accidental swallow. All other error paths in
Normalize(unsupported target,normalizeImagefailures) are unchanged and still propagated.No material error-handling or edge-case issues in this change.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 5m 7s
🪰 Gadfly review —
minimax-m3:cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verified the change against the checked-out code:
media/media.golines 46-152,media/media_test.golines 152-197,llm/content.go(Part interface and constructors),llm/capabilities.go(SupportsImages), and the providers'SupportsImagescall sites. The copy-on-write claim holds:dropOldestImagesalways allocates a newMessagesslice and a newPartsslice for every message containing anImagePart; untouched messages'Partsslices are shared but never mutated. Thenparameter is bounded: callers computen = total - caps.MaxImagesPerReqonly aftercaps.SupportsImages()has confirmedMaxImagesPerReq > 0, son ∈ [1, total-1], and the innerdropped < nguard guarantees termination.VERDICT: No material issues found
Security review (🔒) of this change:
imageOverflowPlaceholderis a package-levelconststring; no caller-controlled bytes flow into it, and no new SQL/command/path/template sinks are touched.image.Decode/DecodeConfigpaths unchanged.n <= 0early-returns; the loop is bounded by bothdropped < nand the finite message/parts walks.caps.MaxImagesPerReqis guaranteed> 0by the earlier!caps.SupportsImages()check atmedia/media.go:58, soncannot exceedtotal - 1. No panic on empty messages.MaxImagesPerReq = 0still means "no image support" (the!caps.SupportsImages()branch is unchanged), and the provider-side count backstop mentioned in the PR description is untouched. The behavior change (silent drop vs. hard error) is a policy/UX choice, not a security regression — a constant string in place of an image does not widen the trust boundary.🎯 Correctness — Minor issues
VERDICT: Minor issues
I read
media/media.go(lines 1–152),media/media_test.go(lines 152–197), and confirmed the helper definitions inllm/message.go,llm/content.go, andllm/capabilities.go.Findings (Correctness lens)
media/media_test.go:194— Mutation assertion is a no-op for this case.The test claims to verify copy-on-write with:
In this test,
req.Messages[0]starts as[a, b]. After the drop, the expected output for that slot is[placeholder, b]— also length 2. IfdropOldestImageswere buggy and mutated the input in place (replacingawith the placeholder text), the length would still be 2 and this assertion would pass silently. The check therefore cannot distinguish a correct CoW result from a mutation that happens to preserve part count.The two nearby assertions (
bytes.Equal(im.Data, a.Data)for kept images, and theplaceholders != 1count) only inspect the output. There is no check thatreq.Messages[0].Parts[0]is stillllm.ImagePartwithData == a.DataafterNormalizereturns.Suggested fix: assert that the first part of the first input message is still an
ImagePartwhoseDataequalsa.Data, e.g.Confirmed by reading the test (lines 152–197) and the loop in
dropOldestImages(media/media.go:116–143), which mutatesout.Messages[mi].Partsin a way that, if done in place on the input, would leavelen(req.Messages[0].Parts) == 2.media/media_test.go:182-192— "Oldest dropped" check is one-sided.The test asserts that the oldest image
a's bytes are absent from the kept images, but does not positively assert thatb's andc's bytes are present. A bug that produced a kept set of[b, b](e.g. a duplicate rather than a different image) would pass all three existing checks:len(imgs) == 2,placeholders == 1, and no kept image equalsa.Data. The PR description promises "the most-recent two … are kept" — the test should pin both ends.(Note: the draft's specific example — "a bug that kept
aandc(and droppedb)" — is actually caught by line 189, since keepingawould puta.Datain the kept set. The valid core concern is that the kept set's identity is not positively asserted.)Suggested fix: pin the kept set explicitly, e.g. after collecting
imgs:Confirmed by reading the test (lines 168–192): only
a.Datais checked; no assertion referencesb.Dataorc.Dataafter the drop.Verified as correct (no issue)
dropOldestImages(media/media.go:116–143) on several message layouts (single message with many images; images spread across messages with intervening text-only turns; cap = 1, 2, ≥ total). The front-to-back single-pass with early-exit afterndrops correctly yields "drop thenoldest images, keep the rest," and does not exceedMaxImagesPerReqin any layout I checked.dropOldestImagesallocates a freshMessagesslice (lines 121–122) and only reassignsout.Messages[mi].Parts(line 140), so the caller'sreq.Messagesand per-messagePartsslices are not touched. The subsequentnormalizeImageloop inNormalize(media.go:74–103) operates on the rebindingreqand preserves CoW for unchanged messages. Verified by reading both functions end-to-end.n <= 0/ invariant onn: BecauseSupportsImages()(llm/capabilities.go:37) requiresMaxImagesPerReq > 0, thetotal > caps.MaxImagesPerReqbranch (media.go:70) only fires with0 < n ≤ total. Then <= 0guard is correct and sufficient.llm.Text(imageOverflowPlaceholder)returnsllm.TextPart(llm/content.go:36), which is a validPart— no interface-mismatch.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
media/media.go:145—hasImagePartis a one-shot helper that doesn't earn a name. Confirmed: grep shows it's defined only atmedia/media.go:145and called only frommedia/media.go:128insidedropOldestImages. Its sole effect is to skip amake([]llm.Part, 0, len(...))allocation for messages with no image part; the inner loop below it already produces a semantically identical result for that case (it would copy through unchanged). Inline the check or drop the early-continue entirely — naming a one-call internal helper makes future readers chase it.media/media.go:116—dropOldestImagesmisnames the operation. Confirmed againstmedia/media.go:112-115: the doc comment says "replaces the n oldest image parts … withimageOverflowPlaceholdertext, keeping the most-recent images and preserving every message's turn structure," andmedia/media.go:107-110says the constant "replaces an image dropped to fit." The package doc atmedia/media.go:12-17also says "replaced with a short text placeholder." The function namedropimplies removal; the parts are preserved (count and turn structure intact, only the bytes change). Suggest renaming toreplaceOldestImages(matches doc text) orelideOldestImages.media/media_test.go:152—TestNormalizeTooManyImages_DropsOldestbreaks the file's naming convention. Confirmed by listing everyTestfunction inmedia/media_test.go: this is the only test in the file using an underscore separator (lines 104, 118, 141, 199, 212, 231, 254, 273, 291, 330, 346, 368, 389, 410, 427, 462, 487, 527). Rename toTestNormalizeTooManyImagesDropsOldestfor consistency.media/media.go:70-104— minor duplicated copy-on-write when overflow and a transform both apply. Confirmed: afterreq = dropOldestImages(req, ...)at line 71,req.Messagesis already a fresh slice (seeout.Messages = make(...)+copy(...)atmedia/media.go:121-122). Then at line 74out := reqaliases that slice header, andcopiedMessagesstartsfalse. If any image subsequently transforms, the loop atmedia/media.go:90-94doesout.Messages = make(...)+copy(out.Messages, req.Messages)again — a redundant copy, since the original input is no longer reachable throughreq. Not buggy (the input stays unmutated either way), just wasted work. Worth knowing if the copy-on-write logic is ever refactored.⚡ Performance — Minor issues
VERDICT: Minor issues
Redundant double-copy on overflow + normalize —
media/media.go:70-103. Confirmed by reading the file end-to-end.dropOldestImages(lines 116–143) unconditionally allocates a freshMessagesslice at line 121 and, for every message that carries an image, a freshPartsslice at line 131. The normalization loop at lines 74–103 then begins without := req(wherereqhas already been reassigned todropOldestImages's output on line 71) but itscopiedMessages/copiedPartsbooleans default tofalseand have no knowledge that copies already exist. As soon as any surviving image'snormalizeImagereturnschanged=true, lines 90–99 re-make/copytheMessagesslice AND re-make/copythat message'sPartsslice — wasted work on the overflow path. Output remains correct (the second copy is independent ofreq), but it is a real regression versus the pre-change behavior where overflow short-circuited to an error before the normalization loop ran. The path is cold (only whentotal > MaxImagesPerReq), so impact is bounded. Suggested fix: havedropOldestImagesreturn a flag (or restructure so the normalization loop readsout.Messagesdirectly) so the second copy is skipped whendropOldestImagesalready produced fresh slices.Pre-existing count pass is unchanged —
media/media.go:47-54. Confirmed: the existing count pass walks every message/parts pair exactly once and is untouched by this diff; the newdropOldestImagesadds another linear O(M·P) pass over the parts, so total work remains linear in the total number of parts — no quadratic behavior is introduced. No action.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling & edge-cases lens, the change holds up. Verified by reading
media/media.goandmedia/media_test.go:n <= 0guard at the top ofdropOldestImages(media/media.go:117-119) absorbs bothn == 0and any negativenthat could result fromtotal - caps.MaxImagesPerReq(e.g., negative cap values). Safe.SupportsImages()is checked before the overflow branch (media/media.go:58), soMaxImagesPerReq == 0with images present still returnsErrUnsupportedand never reachesdropOldestImages— no path drops all images and proceeds to encode an empty list.totalinvariant preserved:n = total - caps.MaxImagesPerReqwith the priortotal > caps.MaxImagesPerReqcheck guarantees1 <= n <= total, so the per-image loop can never underrun the count of availableImageParts.out.Messagesis freshly allocated andcopy'd (media/media.go:121-122); every touched message gets a freshly allocatedPartsslice (media/media.go:131). The laternormalizeImageloop operates on the newout, not the input. The test atmedia/media_test.go:193-196verifies the input is untouched.Parts(no images) is skipped viahasImagePart; emptyMessagesis impossible to reach here becausetotal == 0returns early atmedia/media.go:55-57.Normalize(undecodable bytes, byte-budget failure) are unchanged and still wrapllm.ErrUnsupported.if dropped >= n { break }check is evaluated before processing each message and after every drop, so the exactnoldest images are replaced with text placeholders.One non-blocking observation (not in my lens, so I'm not reporting it as a finding): the placeholder is a
TextPartinserted mid-Partsslice; if any provider's parser assumes images are grouped contiguously or that text precedes images, it may now mis-tokenize a message. That's a provider-contract concern rather than an error-handling one.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 2m 48s
🪰 Gadfly review —
glm-5.1:cloud(ollama-cloud)⏳ Reviewing… this comment will update with findings and run time.
🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-06-28 22:06:26Z
claude-code/opus· claude-code — ✅ doneclaude-code/opus:max· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ doneminimax-m3:cloud· ollama-cloud — ✅ doneragnaros/qwen3.6-27b· ragnaros — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review —
claude-code/opus(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Reviewed through the 🔒 Security lens against the checked-out code (
media/media.go:46-152,llm/content.go:14-39).ImageParts with a fixed compile-time constant (imageOverflowPlaceholder,media/media.go:110) viallm.Text(...). The placeholder text is not derived from any untrusted input, so it introduces no template/prompt-injection vector and no format-string risk (verified:llm.Textis a plain struct constructor,llm/content.go:36).dropOldestImages,media/media.go:116-143) only allocates new slices; no shared-state corruption that could be security-relevant.SupportsImages/MIME/byte-budget/dimension checks (media/media.go:58,normalizeImage) are unchanged, so no validation control was weakened — the provider-side count check still acts as a backstop per the PR description.Nothing in the security lane is materially wrong here. (The behavioral question of whether silently dropping images is desirable is a correctness/design concern handled by another lens, not a security defect.)
🎯 Correctness — No material issues found
I've verified the change against the checked-out code. My correctness analysis:
media.go:70-71,116-143):n = total - MaxImagesPerReq, kept =total - n = MaxImagesPerReq. No off-by-one. Sincen < total, there are always ≥nimages to replace, so it drops exactlyn. Verified.dropOldestImagesiterates messages low→high index and parts front→back, replacing the firstnimages. Chat history is oldest→newest, so the kept images are the newest — matches the stated intent. Verified.SupportsImages()isMaxImagesPerReq > 0(llm/capabilities.go:37), so the cap-0 case still hits the text-onlyErrUnsupportedpath before the overflow branch. Verified.dropOldestImagesallocates a freshMessagesslice and freshPartsslices only for modified messages; unmodified messages share input slices but are never written. The downstream normalization loop (media.go:90-100) copies again before mutating. Input request is never mutated — confirmed by reading both copy sites and matched by the test's mutation assertion.llm.Text(...)returns a validPart(llm/content.go:36) appended into[]llm.Part; test type-switches onllm.TextPart/llm.ImagePart, both real types. Test imports (bytes,errors,strings) all still used elsewhere — no unused-import compile break.VERDICT: No material issues found
Through the correctness lens, the change is sound:
kept == MaxImagesPerReq, no off-by-one), verified atmedia/media.go:70-71and116-143.MaxImagesPerReq == 0(text-only) path is preserved ahead of the overflow branch (llm/capabilities.go:37).No magic constant beyond the count arithmetic, which checks out. Nothing material in my lane.
(Outside my lens, worth a glance by another reviewer: replacing an
ImagePartinside an assistant-role turn with a text placeholder changes that turn's content shape — a provider/error-handling concern, not a correctness bug.)🧹 Code cleanliness & maintainability — Minor issues
I've verified each finding against the actual source in
media/media.go. All three are confirmed:dropOldestImagesallocatesout.Messagesat lines 121-122;Normalizereassignsreqat line 71, then the transform loop re-allocatesout.Messagesat lines 90-94 when any image changes — confirmed double allocation of the Messages slice in the overflow-and-transform case.llm.Text(...)rather than removing it — confirmed.VERDICT: Minor issues
The change is well-documented, the helpers are small and single-purpose, and it follows the file's existing copy-on-write convention.
Findings (code cleanliness lens):
Redundant / split copy-on-write of
Messagesin the overflow path —media/media.go:121-122andmedia/media.go:90-94.dropOldestImagesunconditionally allocates a freshout.Messagesslice and copies into it. Back inNormalize,reqis reassigned to that result, and then the main transform loop (if !copiedMessages { out.Messages = make(...); copy(...) }) copies the same already-fresh slice a second time whenever any image also needs downscale/transcode. So in the overflow-and-transform case theMessagesbacking array is allocated twice. Low severity (small, conditional allocation), but it's the kind of split invariant that's easy to break later. A low-churn fix: havedropOldestImagessignal that it already copied (e.g. return a bool, or haveNormalizesetcopiedMessages = trueafter calling it) so the main loop reuses the copy.dropOldestImagesdoesn't drop — it substitutes —media/media.go:112-143. The name says "drop", but the function replaces each image with a placeholderTextPart(preserving turn structure). The doc comment clarifies, so this is cosmetic, but a name likeelideOldestImageswould match the behavior. Trivial.Image-counting iteration is now duplicated three ways — the "range parts, type-assert
llm.ImagePart" loop appears inNormalize's counter (media.go:48-54),hasImagePart(145-152), and the main transform loop (78-82). Two predate the PR andhasImagePartis a reasonable cheap pre-filter, so no refactor is warranted — noting only that a tinyfunc isImage(p llm.Part) boolwould DRY it up if touched again. Trivial.Nothing in this lens is blocking. The new code reads cleanly and
TestNormalizeTooManyImages_DropsOldestis clear about intent.⚡ Performance — No material issues found
Reviewing through the Performance lens. I've read the full
media.goand the diff.VERDICT: No material issues found
The change is performance-neutral to slightly beneficial under my lens, and I could not find a material efficiency regression.
Analysis (verified by reading
media/media.go:46-152):dropOldestImagesruns before the decode loop (media.go:70-72, before line 76). Dropped images become text placeholders and are therefore never decoded/downscaled/re-encoded bynormalizeImage. Since image decode+encode is by far the most expensive work in this package, dropping overflow images up front reduces CPU and the outbound payload size rather than adding cost. Net win.dropOldestImagesis a single front-to-back pass that early-exits oncenimages are replaced (media.go:124-127). The pre-existing image-count scan (media.go:48-54) is unchanged. The per-Normalize cost remains O(messages × parts); the agent-loop accumulation cost is inherent to growing chat history, not introduced here.Messagesheader slice plus the Parts slices of messages that actually contain a dropped image are reallocated (media.go:121-122,131-140); image byte buffers are shared, not copied.Trivial, non-material observations (flagging for completeness, not as issues — I would not block on these):
media.go:90-100: when overflow and a kept image needs transformation both occur, theMessagesslice (and an affected message'sParts) get reallocated a second time —dropOldestImagesalready did the copy-on-write, then the main loop redoes it because it tracks its owncopiedMessages/copiedPartsflags. This is a duplicate O(len(messages)) allocation, negligible next to image decoding, and only on the overflow+transform path.media.go:128:hasImagePartpre-scans a message's parts, then the rebuild loop scans them again — a double pass, but only across messages untilnimages are dropped. Micro-optimization, not worth changing.Nothing here rises to a real performance regression. The overflow path is exercised rarely and does strictly less expensive work than the old error path would have on a retry chain.
🧯 Error handling & edge cases — No material issues found
I've verified the change against the actual code. Here's my review through the error-handling & edge-cases lens.
VERDICT: No material issues found
I traced every unhappy path the diff introduces and confirmed the edge cases are handled:
Zero / negative cap (the
0 = no image supportcontract). The dangerous case would beMaxImagesPerReq <= 0reachingdropOldestImages(req, total-cap)and dropping every image (or computing a hugen). Verified it can't:Capabilities.SupportsImages()isMaxImagesPerReq > 0(llm/capabilities.go:37), andNormalizereturns the text-onlyErrUnsupportedatmedia/media.go:58before the over-count branch. So0and negatives short-circuit cleanly. ✅nis always positive at the call site. The branch guardtotal > caps.MaxImagesPerReq(media/media.go:70) guaranteesn = total - cap >= 1, anddropOldestImagesadditionally guardsn <= 0(media/media.go:117). No way to under/over-count. Kept count is exactly the cap. ✅Nil / empty parts.
hasImagePart(nil)and the inner loop both range over nil/empty slices safely (media/media.go:128-139, 145-152); messages with no parts are skipped. No panic on empty collections. ✅Drop accounting & boundary. The outer
dropped >= nbreak plus the innerdropped < npredicate correctly stop replacing once exactlynoldest images are gone, even when multiple images live in one message — later images in the same message are retained. No off-by-one. ✅No swallowed errors / no mutation.
dropOldestImagesis total (returns no error, nothing to swallow). Copy-on-write is real:out.Messagesis freshly allocated, and only messages containing images get rebuiltPartsslices, so the caller's request is never mutated — confirmed both by reading the code and by the new test'slen(req.Messages[0].Parts) != 2assertion (media/media_test.go). The subsequent normalization loop's lazy re-copy of the already-fresh slice is redundant but harmless. ✅Interaction with downstream normalization / backstop. After the drop the remaining-image count equals the cap, so the provider-side
<=count backstop still passes; per-image normalization runs unchanged on the survivors. No new failure path introduced. ✅Nothing in the error-handling lane is materially wrong. The diff replaces a hard refuse with a total, side-effect-free transformation, and the one genuinely risky edge (cap ≤ 0) is foreclosed upstream.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 43s
🪰 Gadfly review —
claude-code/sonnet(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The changes are clean from a security perspective. Here's what I verified:
Placeholder injection:
imageOverflowPlaceholder(media/media.go:110) is a hardcoded constant. No user-controlled data is interpolated into the text that gets injected into the model's context. No injection vector.Integer arithmetic:
total - caps.MaxImagesPerReqat line 71 is guarded bytotal > caps.MaxImagesPerReqat line 70, andcaps.MaxImagesPerReq > 0is guaranteed by theSupportsImages()guard at line 58 (verified:llm/capabilities.go:37—SupportsImages()returnsMaxImagesPerReq > 0). No underflow possible.Copy-on-write:
dropOldestImagesallocates a freshMessagesslice and a freshPartsslice for each affected message before substituting any parts. The originalreqargument is never mutated. Verified by reading lines 120–142. The subsequent normalization loop at lines 74–103 applies its own independent copy-on-write on top of the already-copiedreq, which is correct.No new attack surface:
Normalizetakes caller-controlledcapsand caller-controlledreq— both already trusted inputs. The behavior change (drop-oldest instead of refuse) shifts load from error-path handling back into the same normalization pipeline, with no new parsing, no network access, no deserialization, and no credential handling.🎯 Correctness — No material issues found
No material issues found
I traced through the full logic path in the checked-out code:
totalcounting and drop arithmetic (media.go:47–72)totalis counted beforedropOldestImagesis called;n = total - caps.MaxImagesPerReqis the exact surplus. After droppingnimages,total - n == caps.MaxImagesPerReqimages remain. Verified.SupportsImages()guard ordering (media.go:58–72)!caps.SupportsImages()(i.e.MaxImagesPerReq == 0) is checked first and returnsErrUnsupported. The overflow branch is only reached when images are supported, sonis always positive andcaps.MaxImagesPerReq >= 1whendropOldestImagesis called. Verified by readingllm/capabilities.go:37.dropOldestImagessemantics (media.go:116–143)Iterates messages front-to-back (oldest first), replaces the first
nImageParts withTextPartplaceholders, then breaks. Within a mixed-content message the inner loop correctly counts onlyImageParts and leavesTextParts in place. Then <= 0early return is a sound defensive guard. Verified.Copy-on-write chain (media.go:70–103)
After
req = dropOldestImages(req, n), the outer normalization loop reads from the reassignedreq(not the original). For messages wheredropOldestImagesreplaced images, thePartsslice is already a new slice; the normalization loop then copies it again only if a further transformation is needed. No double-mutation hazard. Verified.Test coverage (media_test.go:152–197)
TestNormalizeTooManyImages_DropsOldestchecks: no error, exactly 2 images retained, exactly 1 placeholder,a's bytes absent from retained images, and the inputrequnmutated. Thebytes.Equalona.Datais reliable becausegradient(2,2)vs.gradient(4,4)vs.gradient(8,8)produce structurally distinct PNG payloads. Verified.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code. Here is the corrected review:
Verdict: Minor issues
Finding 1 — Test name breaks the file's naming convention (
media/media_test.go:152)Every other top-level test in this file uses camelCase with no underscores:
TestNormalizeFastPathNoImages,TestNormalizeFastPathFittingImages,TestNormalizeImagesUnsupported,TestNormalizeGarbageBytes,TestNormalizeCopyOnWrite,TestNormalizeInputNotMutated, etc. The new functionTestNormalizeTooManyImages_DropsOldestintroduces a_-separated suffix that stands out ingo test -voutput and is inconsistent with the rest of the file. A consistent name would beTestNormalizeOvercountDropsOldest.Finding 2 —
hasImagePartis a single-use helper that re-walks parts the inner loop already walks (media/media.go:128,145–152)hasImagePartis called once per message as a pre-scan guard insidedropOldestImages(line 128) to skip messages with no images before allocating apartsslice. The type assertion it performs is identical to the condition on line 133 inside the inner loop that immediately follows. Every image-bearing message therefore has its parts iterated twice — once byhasImagePart, once by the replace loop. BecausedropOldestImagesis only invoked when the image count already exceeds the cap andnis typically small, the cost is negligible; but the helper introduces a second maintenance point ifllm.ImagePartever becomes an interface rather than a concrete type.A lazy-alloc inner loop would eliminate both the double scan and the helper:
Not blocking, but the current structure is worth a note.
⚡ Performance — No material issues found
No material issues found
Verified the new
dropOldestImagespath in full:Double Messages-slice copy (overflow + image transforms): When both conditions hold —
total > MaxImagesPerReqAND the surviving images also need normalization — the Messages slice is allocated twice: once indropOldestImages(line 121) and again in the COW path of the main loop (line 91). That's O(N) in message count on a path already dominated by image decode/scale/encode work. Not material.hasImagePartprecheck (lines 128–130): Parts in a message with images are walked twice (once inhasImagePart, once in the replace loop). Given the alternative (allocating apartsslice for every message regardless), the precheck is net-beneficial. For 1–4 parts per message, the extra scan is negligible.No unbounded growth, no quadratic behaviour:
dropOldestImagesis O(messages × parts), both of which are small and bounded. It's called at most once perNormalizecall, which itself is called once per chain-target attempt, not inside a tight hot loop.The performance profile of the overflow path is indistinguishable in practice from the pre-existing image-normalization path: both are dominated by image I/O (decode config, downscale, re-encode), not by slice bookkeeping.
🧯 Error handling & edge cases — No material issues found
No material issues found
I verified the following paths through the lens of error handling and edge cases:
dropOldestImagesinvariants (verified by readingmedia/media.go:116-143):n <= 0guard at line 117 is safe; the call site at line 71 always passestotal - caps.MaxImagesPerReqwhich is≥ 1because we only enter the branch whentotal > caps.MaxImagesPerReqandMaxImagesPerReq ≥ 1(enforced bySupportsImages()at line 58).nis always≤ total - 1(sinceMaxImagesPerReq ≥ 1), so there are always more images inreqthan we need to drop.droppedwill reachnwithout the loop running out of images.TextParts — still a well-formed message, turn structure preserved.hasImagePart(nil)is safe (empty loop) for messages with nil Parts.Copy-on-write soundness (verified by reading lines 120-143):
out.Messagesis a new slice before anyout.Messages[mi].Partsassignment; assigningout.Messages[mi].Parts = partswrites only into the new slice, leavingreq.Messages[mi]untouched.req.Messages[0].Partslength is still 2 after normalization.Post-drop normalization loop (verified by reading lines 74-104):
dropOldestImagesreturns, replaced image parts areTextPart. The inner loop at line 79 (if _, ok := part.(llm.ImagePart); !ok { continue }) correctly skips them, so placeholders are never passed tonormalizeImage.copiedPartstriggers a freshcopy(parts, req.Messages[mi].Parts)before any in-place write, capturing the post-drop parts (with placeholders intact) into the new slice.pifromfor pi, part := range req.Messages[mi].Partsis stable becausedropOldestImagesreplaces each dropped image with exactly oneTextPart, so slice lengths are unchanged.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 3m 21s
🪰 Gadfly review —
deepseek-v4-pro:cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The change is a policy shift (drop-oldest vs. refuse) implemented through straightforward slice manipulation and a hardcoded constant placeholder string. From a security lens:
"[earlier image omitted to fit this model's per-request image limit]"is a compile-time constant, never derived from user input. It's inserted as aTextPart— plain conversation text with no path/command/template interpretation.Normalizeis a pure media-transformation function with no access-control decisions.dropOldestImagesonly does type-assertion switches on the sealedllm.Partinterface (TextPart/ImagePart) and copies slices. No new parsing, no URL fetching.normalizeImagepipeline (decode → downscale → re-encode), which already handles potentially malicious image bytes. Dropped images are discarded entirely — their bytes are never decoded or forwarded.dropOldestImagesis only reachable whenMaxImagesPerReq > 0(guarded by theSupportsImages()check at line 58) andtotal > MaxImagesPerReq, sonis always positive and bounded by the actual image count. Then <= 0early-return is defensive but not strictly necessary given the call site.The one design-level observation (not a security bug): silently dropping content changes what the model sees, which could be exploited by an adversary who intentionally floods a request with decoy images to displace earlier ones. But that's a functional/correctness concern (does the model still answer correctly?), not a security vulnerability — the model receives an explicit placeholder, and the attack surface (crafting many images to push others out) already existed in the "refuse" path (DoS via rejection). This is outside the security lens.
🎯 Correctness — No material issues found
Verdict: No material issues found
I traced through the full logic of
dropOldestImages, its integration intoNormalize, the copy-on-write interaction with the existing normalization loop, and the test coverage. The implementation correctly:nimages (front-to-back across messages, then left-to-right within each message's parts)ImagePartwith aTextPartplaceholder, keeping part count identical)Messagesslice allocated; newPartsslices for modified messages)ImagePartinstances intact for the downstream normalization loop, which correctly skips the placeholderTextPartinstances via itsip, ok := part.(llm.ImagePart)guardn <= 0(no-op),n >= totalcan't happen becauseSupportsImages()gates outMaxImagesPerReq == 0before this path, andtotal > caps.MaxImagesPerReqensuresn < totalThe double-copy of
Messagesthat can occur (once indropOldestImages, again in the normalization loop when an image actually needs transformation) is wasteful but not a correctness issue — it's an allocation concern outside my lens.🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
The change is clean and well-structured through the 🧹 lens:
dropOldestImagesis a focused, self-contained helper with clear copy-on-write semantics, a defensiven <= 0guard, and a straightforward single-pass algorithm. The unexportedhasImageParthelper is a minor extraction (used once, 6 lines) — a matter of taste, not a material problem.imageOverflowPlaceholderis properly named and documented, avoiding a magic string literal in the loop body.TestNormalizeTooManyImages_DropsOldest), exercises the three key properties (correct count kept, placeholder inserted, input unmutated), and uses distinguishable image sizes so the "oldest dropped" assertion is meaningful.dropOldestImageshandles its own copy, and the main loop's!ok { continue }naturally skips the inserted text placeholders.One minor observation (not worth a finding on its own):
hasImagePartcauses a double-scan of parts for messages that contain images — once to decide whether to enter the block, then again in the inner loop. The same effect could be achieved by building the new parts slice lazily (only allocating when the first image-to-drop is encountered), but the current approach is clear and the slices are tiny; this is not a real maintainability concern.⚡ Performance — Minor issues
Verdict: Minor issues
Messagesslice when overflow + transform coincide (media/media.go:70–71+media/media.go:90–93).dropOldestImagesallocates a freshout.Messagesviamake+copy(line 121–122). Back inNormalize,out := req(line 74) shares that slice. If any kept image later needs transformation (downscale/transcode), the normalization loop allocatesout.Messagesa second time (line 91), discarding the first. In practice the wasted allocation is dwarfed by the decode/scale/encode work that triggers it, and[]llm.Messageis a handful of words per entry, so the impact is negligible. Still, it's avoidable:dropOldestImagescould return a flag indicating it already copied, lettingNormalizeskip the redundant allocation. (Verified by readingmedia/media.go:116–143andmedia/media.go:74–104.)No other material performance regressions: the new
dropOldestImagespath is O(messages + parts) — same order as the existing normalization loop — with no N+1 queries, no quadratic behavior, no unbounded growth, and no blocking calls on hot paths. ThehasImagePartpre-scan before the inner loop is a constant-factor double-traversal of parts within a single message, which is immaterial given typical part counts.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
I traced every unhappy path through
dropOldestImagesand its integration intoNormalize, verifying against the actual checked-out code. The error-handling and edge-case behavior is sound:n <= 0guard (media.go:117): returns early, no-op. The call site only passesn ≥ 1(guarded bytotal > caps.MaxImagesPerReqat line 70), but the defensive check is correct.MaxImagesPerReq == 0with images present: caught earlier bySupportsImages()at line 58 — we never reach the overflow branch. Verified by readingllm/capabilities.go:37.Partsslices:hasImagePartandrangeover nil both handle these safely — no panic. Verified by reading the loop at lines 128–140.hasImagePartreturns false, message skipped,Partsslice left shared with original (correct copy-on-write). Verified.dropped >= n, keeps the rest. Correct.TextPart, notImagePart: the subsequent normalization loop at lines 78–82 skips them via the.(llm.ImagePart)type assertion. They won't be fed tonormalizeImage. Verified by readingllm/content.go:36(Text()returnsTextPart).normalizeImagereturns an error,Normalizereturnsllm.Request{}, err— the partially-built output (with placeholders) is discarded. No leaked state.dropOldestImagestakesreqby value, creates a newMessagesslice, and only replacesPartsslices for messages where images were actually dropped. The test at line 193–196 confirms the original request'sPartscount is unchanged. Verified by reading the function at lines 116–143.totalcomputed before mutation:totalis counted from the originalreq(lines 47–53), thendropOldestImagesis called with the originalreq(line 71), thenreqis shadowed. No inconsistency between the count and the drop target.No ignored errors, no missing cleanup, no panics on bad/empty input, no off-by-one in the drop count, no nil-pointer hazards. The double-allocation of
Messages/Partswhen bothdropOldestImagesand the normalization loop copy is a minor performance waste, not an error-handling concern.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 6m 7s
🪰 Gadfly review —
claude-code/opus:max(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I reviewed this change strictly through the 🔒 Security lens, reading the full implementation in
media/media.go:46-152and the relevantllmtype definitions (llm/content.go).Findings:
imageOverflowPlaceholder(media/media.go:110), a compile-time constant. No untrusted/tainted data (image bytes, MIME, or any caller-supplied string) is reflected into the placeholder or any other sink. Verified the placeholder isconstand thatdropOldestImagesonly ever appendsllm.Text(imageOverflowPlaceholder)— never anything derived from the dropped image's contents (media/media.go:131-138).ImageParts are discarded, not decoded, parsed, or echoed. The remaining (most-recent) images still flow through the existing unchanged normalization pipeline (normalizeImage, byte/MIME/dimension checks) atmedia/media.go:74-104, so no validation is bypassed for the images that survive.One note adjacent to (but still within) the lens: switching from "refuse" to "silently drop oldest images + placeholder" means any upstream consumer that relied on
ErrUnsupportedas a signal that content was being altered no longer gets it. The PR documents the provider-side count check remains as a backstop, and I confirmed nothing in this diff weakens an authorization or input-validation control — so this is a behavior/contract concern (other lenses' territory), not a security defect.Nothing in my lane is materially wrong here.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the drop-oldest logic through the correctness lens, verifying against the checked-out code (
media/media.go,llm/capabilities.go,llm/content.go).Checks performed and why they hold:
No "drop all images" when
MaxImagesPerReq == 0.dropOldestImagesis called withn = total - caps.MaxImagesPerReq. I confirmedSupportsImages()isMaxImagesPerReq > 0(llm/capabilities.go:37) and the!caps.SupportsImages()guard (media.go:58) returns before line 70. So the overflow branch only runs with cap ≥ 1, droppingtotal-capand keeping ≥ 1 image. No path drops every image."Most recent kept" is correct, no off-by-one. With
total=9, cap=8,n=1; iterating messages front-to-back (oldest→newest) drops the firstnImageParts and keeps the trailingcap. The innerdropped < nguard plus the outerdropped >= nbreak correctly handle a single message holding more thancapimages (drops the leading ones, keeps the trailing ones within that message). Resulting kept count is exactlycap, which satisfies the provider backstop's>check.Copy-on-write preserved; original request untouched.
dropOldestImagesallocates a freshout.Messagesand freshPartsslices only for messages it edits; the caller'sreq.Messagesslice and its parts are never written. The subsequent main-loop COW operates on the post-dropreq, so indices stay consistent (the part count changes happen before the normalize loop reads them). Verified the test's mutation assertion (req.Messages[0].Partslength) is a genuine guard.Counting consistency.
totalis counted overImageParts across all messages (media.go:48-54); replacing dropped images withTextPartplaceholders means the post-drop image count is deterministicallycap. No risk of under/over-dropping.Compiles.
bytesis already imported inmedia_test.go:4;llm.Text,llm.Image,llm.ImagePart,llm.TextPartall exist as used.Non-correctness observation (not flagging as a finding): when several images in one message are dropped, each becomes a separate identical placeholder
TextPart, so a message can carry repeated placeholder lines — verbose but behaviorally correct.The change does what it claims; I found no logic, unit, boundary, or magic-number error in my lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings check out against the code. Confirmed: the test file's
errors/bytes/stringsimports all remain in use (lines 144, 189, 176, etc.), so the test-rewrite does not break compilation. On finding 1, only the package doc (12–17) and the inline comment (61–69) actually restate the same rationale; thedropOldestImagesdoc comment (112–115) describes mechanics, not the chain-exhaustion justification — so it's a duplication, not a triplication. Finding 2's double Messages-slice copy is verified by tracing line 71 → 121 → 90–93.VERDICT: Minor issues
Reviewed through the code-cleanliness & maintainability lens. The change is small, well-named, and follows the existing copy-on-write idiom in
media.go. Type assertions match patterns used elsewhere; test imports all still used (errors/bytes/stringsat media_test.go:144,189,176 etc.). Two minor maintainability points:Duplicated rationale comment (
media/media.go:12-17and61-69). The same justification — a hard refuse exhausts a chain whose targets share the same cap, the agent loop accumulating a preview image per iteration, MaxImagesPerReq stays the per-model knob — is spelled out in full in both the package doc and the inline branch comment. (ThedropOldestImagesdoc at 112-115 covers mechanics, not this rationale.) Two copies that must move together is a drift risk. Low-churn fix: keep the policy statement in the package doc and trim the inline comment at 61-69 to one line (e.g.// Over-count: keep most-recent MaxImagesPerReq, elide older ones — see dropOldestImages.).Redundant Messages-slice copy in the overflow path (
media/media.go:71→116-122, then90-94). When over-count and some image also needs transform,dropOldestImagesallocates+copiesout.Messagesonce (line 121), then the normalize loop allocates+copies it a second time (lines 90-93) becausecopiedMessagesstarts false on the reassignedreq. Two COW mechanisms with different idioms (make(0,len)+append vsmake(len)+copy+index) now coexist for the same "substitute parts in a message" job. Functionally correct; just a structural redundancy worth a conscious choice rather than overlooked.Neither blocks merge. The naming nit that
dropOldestImagesreplaces (not literally drops) images is acceptable since the doc comment and PR make the placeholder behavior explicit.⚡ Performance — No material issues found
I've reviewed the actual checked-out code. Let me trace the performance characteristics of the new overflow path.
Analysis through the Performance lens:
The change adds
dropOldestImages(media.go:116-143), invoked only on the overflow branch (media.go:70-72). Tracing the cost:dropOldestImagescopies theMessagesslice header (shallow, O(#messages)) and, for each message untilnimages are replaced, scans its parts twice — once inhasImagePart(line 128) and once in the rebuild loop (line 132). Two linear passes over part headers, bounded by message/part counts that are inherently small. No quadratic behavior, no per-image data copies — placeholders are a shared constant string.Textparts, so the subsequentnormalizeImageloop (media.go:76-103) skips them (thepart.(llm.ImagePart)assertion fails). The overflow path therefore does less expensive decode/downscale/re-encode work than before, not more.Messagesslice header is allocated twice (once indropOldestImages, once in the copy-on-write at media.go:91). This is a shallow header copy on a cold path (only when a target both overflows and needs image transformation) — not material.hasImagePart+ the rebuild loop double-scan parts, but only for messages on the cold overflow path. Merging them would be a micro-optimization not worth the readability cost.No N+1, no unbounded growth, no new work in a hot loop, no avoidable large allocations. The per-target recompute matches the pre-existing per-target
totalcount, so there's no chain-level regression.VERDICT: No material issues found
Nothing in the Performance lens is materially wrong. The overflow handling is linear in message/part count, runs only on the cold overflow branch, and actually reduces downstream image-processing work by converting dropped images into trivial text placeholders before the decode/encode pass. Verified by reading
media/media.go:46-152and confirming the placeholder parts are skipped by thenormalizeImageloop's type assertion.🧯 Error handling & edge cases — No material issues found
I've verified the implementation against the checked-out code. Let me trace the unhappy paths and edge cases carefully.
Verification performed:
MaxImagesPerReq == 0(no negative/zero math):SupportsImages()isMaxImagesPerReq > 0(llm/capabilities.go:37). So line 58 returns the text-only error before the overflow path. By line 70,MaxImagesPerReq >= 1, andtotal > capguaranteesn = total - cap >= 1. No zero/negative drop count reachesdropOldestImages, and it guardsn <= 0defensively anyway (media.go:117). ✅nlarger than available images: Can't occur (n < totalalways), but even if it did the loop simply replaces every image and stops — no panic, no index error. ✅total == 0returns early (line 55). A message with no/empty parts hitshasImagePart→ false and is skipped (media.go:128). A message whose parts are all images becomes all-placeholders, never an emptyPartsslice. ✅dropOldestImagesallocates a freshMessagesslice and a freshPartsslice only for image-bearing messages; no-image messages share the originalPartsbut are never written (the second loop only writes onImagePartmatches). InNormalize,copiedMessagesalways triggers beforecopiedParts, so writes land in freshly-copied slices — the originalR0and intermediateR1are never mutated. Matches the test's mutation assertion. ✅dropOldestImagesis pure and cannot fail; nothing to swallow. If a kept image later failsnormalizeImage,Normalizestill returns the wrappedErrUnsupported(line 85) and the dropped-image request is discarded — correct. ✅n; thedropped < nguard stops mid-message and appends the rest, so the most-recentcapimages are retained. Boundary is correct. ✅VERDICT: No material issues found
Through the error-handling & edge-cases lens, the new overflow path is sound:
n <= 0guard indropOldestImagesis correct defensive coding even though the one caller can't reach it.len(req.Messages[0].Parts) != 2check).normalizeImageerror handling is unchanged.The drop-oldest semantics (silently eliding images rather than refusing) is a deliberate, author-acknowledged behavior trade-off — that belongs to the correctness/design lens, not error handling, so I won't restate it here.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 2m 4s
🪰 Gadfly review —
ragnaros/qwen3.6-27b(ragnaros)Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found (security lens).
The change replaces a hard-refusal on image over-count with a drop-oldest-and-replace-with-placeholder strategy. Reviewed through the security lens:
"[earlier image omitted to fit this model's per-request image limit]") is a hardcoded constant, not derived from user input. It cannot be manipulated for prompt injection, template injection, or any other injection attack.ImagePart.Dataholds raw bytes (not URLs), and the new code only iterates, counts, and replaces parts — no decoding, no fetches, no external calls. The package comment explicitly states "majordomo does not download remote content."dropOldestImagescreates a newMessagesslice (line 121) and newPartsslices for affected messages (line 131), so the originalreqis not mutated. Verified against the checked-out code.caps.SupportsImages()gate (line 58, which checksMaxImagesPerReq > 0) runs before the overflow logic. A zero cap correctly rejects withErrUnsupportedrather than silently processing.n <= 0early return (line 117) prevents any pathological negative-count behavior.🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 5: context deadline exceeded
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
media/media.go:71–103— Dual copy-on-write paths don't compose. When a request both overflows and needs per-image transforms,dropOldestImageseagerly allocates a freshMessagesslice (lines 121–122), then the normalization loop inNormalizeindependently checkscopiedMessages(stillfalse) and allocates/copiesout.Messagesagain (lines 90–93). The messages slice is allocated and copied twice with no sharing. Confirmed by reading lines 71, 74, and 90–93.media/media_test.go:176— Test matches placeholder by substring rather than constant.strings.Contains(v.Text, "omitted")is fragile: renamingimageOverflowPlaceholderwould silently make this test pass for the wrong reason. Should reference the constant directly:v.Text == imageOverflowPlaceholder. Confirmed by readingmedia_test.go:170–180.⚡ Performance — No material issues found
Verdict: No material issues found
Through the Performance lens, the changes are clean:
dropOldestImagesfunction does one linear pass with early exit (breakat line 125), andhasImagePartis O(parts) per message but only invoked on messages before the drop quota is filled.dropOldestImagesallocates a newMessagesslice and per-messagePartsslices only for affected messages (copy-on-write). The main normalization loop below it also uses copy-on-write, so worst-case when both overflow and image normalization hit is two rounds of allocation — not material for the described use case (agent loops with ~10 images).dropOldestImages+ main normalization loop (lines 76–102) means three full passes over the request on overflow. For very large requests this is a constant-factor overhead, but still O(n) — not quadratic or otherwise pathological. If requests routinely carry hundreds of messages, consider folding the count intodropOldestImagesor merging it with the normalization pass. For the stated use case, this is negligible.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found (error handling & edge cases lens)
Verified by reading
media/media.go(full file),llm/capabilities.go, andllm/content.go:MaxImagesPerReq == 0path: Correctly guarded bycaps.SupportsImages()at line 58 (MaxImagesPerReq > 0), which returnsErrUnsupportedbefore the overflow branch is reached. Confirmed viagreponSupportsImages()definition.n <= 0guard indropOldestImages: Line 117 handles this cleanly, returningrequnchanged.total - caps.MaxImagesPerReq: Cannot happen — the subtraction only executes undertotal > caps.MaxImagesPerReq, son >= 1.n = total - caps.MaxImagesPerReqand we've establishedcaps.MaxImagesPerReq >= 1, son < total. The loop will always find enough images.Partsslices:hasImagePartreturnsfalsefor nil or empty slices (range over nil is a no-op in Go), so such messages are safely skipped.Messages:totalwould be 0 (range over nil is a no-op), triggering the early return at line 55.dropOldestImagescreates a newMessagesslice viamake+copy, and replacesPartsfor affected messages. The original request's slices are not mutated. The subsequent normalization loop inNormalizeoperates on the already-copiedreq(reassigned at line 71), so the caller's original is preserved.One minor observation (not a bug): the test's mutation check at
media/media_test.go:194only verifieslen(req.Messages[0].Parts) != 2, which is weak — a true in-place mutation would also yield length 2 (one image replaced by one placeholder). A stronger check would verify the first part is still anImagePart. This doesn't affect production code correctness.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 27m 14s