fix(core): bound oversized images returned by MCP tools - #10835

1 Critical (merge-blocking): the sharp load moved behind the file checks, flipping error precedence and turning the Test lane red at head

Where — packages/core/src/utils/image-view.ts. The refactor extracts loadSharp() and calls it from the new prepareImageBuffer() (:214), which prepareImage() now reaches only after fs.stat, the directory / regular-file / 100 MB checks and fs.readFile (:164-206). Before this PR, prepareImage() loaded sharp as its very first step.

Trigger — any image path where one of those pre-load checks fails, on a host where sharp cannot be imported. That is exactly the case zoom-image.sharp-failure.test.ts exists to cover (its own comment: musl Linux without @img/sharp-linuxmusl-x64). The renderer_unavailable ImageViewError is no longer raised first, so the file-level error wins.

Impact — src/tools/zoom-image.sharp-failure.test.ts:54 fails, so Test (ubuntu-latest, Node 22.x) is RED at head a6b22ca2 (run 33640505166 / job 100282204426): expected { …(2) } to match object { type: 'read_content_failure' }, received file_not_found. The test file is untouched by this diff. Behaviour-wise, zoom_image on a host without sharp now reports a missing/invalid file instead of telling the user that image rendering is unavailable — losing the recoverable-error contract that test encodes.

Reproduced locally and A/B-proven:

• head a6b22ca2 in a clean git archive tree → FAILS with exactly the CI assertion (file_not_found vs read_content_failure, same :54:26 frame).
• base main (bde667f8f8), same command → Test Files 1 passed, Tests 1 passed.
• mutation at head, changing nothing else — one line, await loadSharp(); immediately after signal.throwIfAborted(); at the top of prepareImage → zoom-image.sharp-failure, zoom-image, image-view and mcp-tool all pass: 143/143, including this PR's own new tests (bounds an oversized image returned by an MCP tool, forwards an image the renderer cannot bound unchanged, and the three new image-view.test.ts cases).

Fix direction — load the renderer before the file-level checks (the one-line mutation above, or hoist loadSharp() into prepareImage and pass the instance down to prepareImageBuffer). If the new precedence is deliberate — a missing file arguably should say "not found" even when sharp is broken — then zoom-image.sharp-failure.test.ts must be updated in the same PR, and that is a contract change worth a maintainer's call, because the test documents behaviour on hosts without the native binary.

Not attributed to this diff: the same lane also reports coordination-harness.test.ts > forwards final text after an interim leader message and shellReadOnlyChecker.test.ts > handles adversarial rule inputs without regex backtracking. Both files are untouched here and both are timing / CPU-budget sensitive under CI contention, so I did not treat them as this PR's failures.

Checked and clean (so it does not need re-litigating): the ImageViewError catch in boundInlineImageParts correctly forwards unbounded images — GIF/animated, sharp unavailable, over the 100 MB source limit; in-budget images keep their original bytes, format and alpha (boundImageBuffer returns null); audio parts are excluded by the image/ MIME guard; and both MCP call paths plus the buildMcpToolError image path get the same treatment, so no route into the conversation is missed.

LGTM. qqqys's Critical is fixed at this head: prepareImage() loads sharp before fs.stat, so a host without the native binary still reports renderer_unavailable rather than file_not_found. GIF / undecodable images forwarded unbounded is the documented limitation, not a regression. My vote does not dismiss the earlier CHANGES_REQUESTED — that's still qqqys's to clear.

The image-budgeting flow and the previous sharp error-precedence fix look correct at this head. I left one P1 inline: MCP-controlled multi-image results are decoded and rendered with unbounded concurrency; processing them sequentially is the smallest safe fix.

Critical-only review at 636d410a, base c502f3dc.

Verdict: COMMENT — one blocking issue stands at this head. I verified its three load-bearing facts in the current code rather than adopting the round that filed it. The bounding logic itself is well built and I set out what I checked below, so the scope of the problem is clear.

Still standing: the bound withholds images that omni delivery is configured to upload by reference

boundInlineImageParts applies a pre-decode source clamp before the renderer is ever asked to look at the bytes:

const sourceLimitedPart = clampInlineMediaPart(part, IMAGE_MAX_SOURCE_BYTES, …);
if (sourceLimitedPart !== part) {
  boundedParts.push(sourceLimitedPart);
  continue;
}

Anything over IMAGE_MAX_SOURCE_BYTES becomes a text placeholder there. That is the right guard in isolation — it stops unbounded bytes reaching the decoder — but it runs inside the tool, and the omni delivery funnel consumes the tool's output afterwards. Three facts, each checked at this head:

• The tool is omni-blind. A case-insensitive search of packages/core/src/tools/mcp-tool.ts at 636d410a returns zero occurrences of omni. boundInlineParts is called from four sites — its own definition plus the two success paths and buildMcpToolError — with no conditional anywhere on delivery mode. So the clamp is unconditional.
• Omni's own ceilings are higher than the clamp. omni/tool-result-media.ts sets MAX_UPLOADS_PER_TOOL_RESULT = 8 and MAX_UPLOAD_BYTES_PER_TOOL_RESULT = 128 * 1024 * 1024, with the budget loop at lines 71-72; omni/guard.ts sets DEFAULT_OMNI_MAX_UPLOAD_FILE_BYTES = 1024 * 1024 * 1024. An image between 100 MiB and those ceilings is exactly what the funnel exists to deliver, as a fileData reference rather than inline bytes.
• The base did not bound at all. At the merge base this file contains no bounding symbol, so the part reached the funnel intact. The placeholder is therefore new behaviour, not a pre-existing limit being documented.

Net effect: an omni-active session (omni enabled, trusted folder, resolved upload channel) whose MCP tool returns an image above the decoder's source cap now receives a text placeholder where it previously received the image by reference. The direction is fails-closed — nothing is corrupted and the model is told the media was omitted — but it is a functional regression on a supported configuration, and it removes precisely the large-media case that reference delivery was built for.

What makes this a finding rather than an accepted trade is the asymmetry in how the two halves of the interaction were recorded. The lossy-derivative half is deliberately accepted: the description's Risk & Scope covers it and the round-5 scope ledger lists an omni-specific bypass under non-goals, with the reasoning that a smaller derivative consumes less of the funnel's own budget. That is a defensible decision and I am not reopening it. The withheld half has no recorded rationale, and with zero mentions of omni in the file there is nothing in the code to tell the next reader the interaction was considered. Every thread on the PR is marked resolved, which under the standard applied here is not evidence of a fix — and the two commits since the round that filed this are both merges from main, so the code it anchors is unchanged.

Two ways forward, either of which I would read as closing it: restore a narrow carve-out so a part the funnel would upload is not source-clamped first — the round-4 finding is a warning about how not to shape it, since a per-config gate is wrong when the funnel's decision is per-part, so the carve-out has to be per-part or the inline path reopens unbounded; or keep the unconditional bound and record the decision where it can be seen, in Risk & Scope beside the derivative trade and in a comment at the clamp naming omni's higher ceilings. The second is a documentation change and is the cheaper of the two, but it should be a decision rather than an omission.

What I verified as sound

The bounding itself is careful, and the details are worth recording because they are the reason this review is one finding rather than several.

One ceiling read for both decisions. getMaxInlineMediaBytes() is read once into inlineByteCeiling and used for both the renderer's adopt decision and the trailing clamp, so the two cannot disagree within a single result if settings change mid-loop.

The image predicate is the shared one. The gate uses isImagePart(part), the same predicate the vision bridge and getMcpErrorImageContent use, so this bound cannot drift from the repository's definition of "image" and MCP audio blocks — which carry an identical inlineData shape — stay excluded. Untyped application/octet-stream resource blobs are admitted deliberately, because MCP makes a resource's mime optional and transformResourceBlock defaults it, so an image can arrive unlabelled; the renderer sniffs the real format and a genuine non-image falls through the fail-open below.

The placeholder wording is corrected for this call site. oversizedMediaPlaceholder gains optional limitLabel and remedy, and the MCP callers override both. That matters: the shared default tells the model to "reference it via an @file path so it can be read from disk", which cannot exist for bytes that live only inside an MCP response. The untyped-blob case gets its own wording again, because at the pre-decode clamp the real format is not yet known and describing a non-image as an image would be a second false statement. Both changes are backwards-compatible — the parameters are optional and the defaults reproduce the previous text exactly.

Errors are split by persistence, and only the expected kind is swallowed. if (!(error instanceof ImageViewError)) throw error; rethrows anything unexpected rather than hiding it. Within ImageViewError, renderer_unavailable logs at warn because a renderer that cannot load is a host condition that will fail every image of every call, while single-image decode failures stay at debug.

The fail-open is scoped. A part the renderer could not decode is forwarded as the server sent it, and the trailing clamp is applied only to what isImagePart recognises afterwards — so an unlabelled blob that turns out not to be an image is not placeholdered as one.

The image-view.ts refactor preserves an error priority that was easy to lose. Extracting loadSharp() and calling it ahead of the fs.stat in prepareImage keeps an unavailable renderer reporting renderer_unavailable rather than file_not_found, which is what the existing sharp-failure suite pins. The new prepareImageBuffer shares the same source cap, decode, format, page-count and abort checks, with signal.throwIfAborted() retained at each stage and the file path replaced by a caller-supplied label so MCP bytes get a meaningful subject instead of a path that does not exist.

The envelope rewrite is defensive. When the renderer re-encodes to a different mime, the preceding text envelope that announced the original is rewritten, but only when boundedParts.at(-1) is a text part whose content ends with the exact mime-type: <original>] string; otherwise nothing is touched and no exception is possible.

CI

13 checks pass and 7 are skipped at this head with one lane pending and nothing failing. I did not wait on it. No lane exercises an omni-active session with an over-cap MCP image, which is why the suite is green while the item above is open.

Requesting changes at 636d410 on one item: a Critical the author confirmed real, which I have now verified myself at this head, and which is still unfixed. Required CI is green here, with review-pr excluded as reviewer-bot infrastructure per my standing disclosure, and I found no new Critical of my own in the code this PR adds.

Blocking: an image the omni funnel would have delivered becomes a text placeholder

Verified at this head rather than taken from the thread:

• mcp-tool.ts contains no reference to omni anywhere. A case-insensitive count over the file returns zero, so nothing in the bounding path knows whether the omni funnel is about to take the bytes.
• mcp-tool.ts:1383-1385 runs clampInlineMediaPart with IMAGE_MAX_SOURCE_BYTES before decoding, and image-view.ts:15 sets that constant to 100 MiB.
• omni/tool-result-media.ts:33 sets MAX_UPLOAD_BYTES_PER_TOOL_RESULT to 128 MiB, so a part sitting between the two would have been admitted by the funnel and delivered as a fileData reference.
• fileUtils.ts:1413-1423, untouched by this PR, already states the rule for this exact constant and gates it on omniModule === undefined: the 100 MB source cap protects the overview decoder, so it applies only when the overview will actually decode, and gating it where omni takes the file would reject a 150 MB PNG while delivering a 500 MB GIF purely on whether the format has an overview renderer. The MCP path fires the same cap unconditionally, on bytes that with omni active are never decoded at all.

The direction is fail-closed, so this is a lost capability rather than a leak, and the window between the two ceilings is narrow. It is still a regression against base, where this tool applied no clamp and the funnel saw the original bytes, and the tree already carries the precedent for the correct behaviour. Note also that the current head is a merge of main with no code change since the commit at which the author confirmed the finding, so nothing here has moved.

I am not asking this PR to reverse its own recorded non-goal on my authority. Two resolutions, either acceptable:

1. Exempt both withholding clamps when omni delivery is active, following the fileUtils precedent. The author has already established why a half-measure fails: dropping only the pre-decode clamp leaves boundImageBuffer throwing source_too_large above 100 MiB, the catch forwards the part unchanged, and the trailing inline clamp placeholders it anyway. So it is both clamps or neither.
2. Or have a maintainer who is not the author rule in #12471 that the loss is accepted, and record that ruling in this PR. Risk & Scope currently documents only the other half of the omni interaction, namely that the funnel uploads bounded bytes instead of the source-resolution original. It does not mention the withheld-image case and does not link the issue, so a reader merging this today would not learn that a large image becomes a placeholder where base delivered it.

Under either resolution the PR body should name the accepted behaviour and link #12471, so the tradeoff is discoverable from the change itself rather than only from a review thread.

What I checked and am not blocking on

The generalisation of the shared inline-media clamp is done carefully: both new parameters are optional and appended, and with no options supplied the placeholder text is byte-identical to what it was, so no existing caller changes behaviour. The reason for allowing an override is the right one, since the default remedy tells the model to reference a file by an at-path, which cannot exist for bytes an MCP server returned. The mime sanitisation for an untrusted server is retained.

The Suggestions carried from earlier rounds, and the probes deferred under the convergence posture, are not mine to reopen at round six, and consistent with the repository guidance on long-lived PRs I am not adding new ones.

Tier: Deep (new async image-processing path; cross-binary dependency on sharp; error-precedence constraint that is load-order-sensitive; bytes that exist only in an MCP response — no @file fallback)

Verdict: COMMENT — one blocker at this head (R1-1), verified independently below

Findings ledger

R1-1 — BLOCKER: boundInlineImageParts is omni-blind

Location: packages/core/src/tools/mcp-tool.ts, boundInlineImageParts function

Mechanism: The pre-decode source clamp

const sourceLimitedPart = clampInlineMediaPart(
  part,
  IMAGE_MAX_SOURCE_BYTES,
  ...
);
if (sourceLimitedPart !== part) {
  boundedParts.push(sourceLimitedPart);
  continue;
}

runs unconditionally for every image part, regardless of delivery mode. When sourceLimitedPart !== part (i.e. bytes > IMAGE_MAX_SOURCE_BYTES ≈ 100 MB), a {text: "..."} placeholder is pushed and the loop advances. The omni delivery funnel, which would upload such an image by reference rather than inline, only sees what boundInlineParts returns — a text part. The upload path is unreachable.

Three load-bearing facts, verified at head 636d410a:

1. Zero omni awareness in this file. A case-insensitive search of the file returns no occurrences of omni. The clamp is unconditional on delivery mode — there is no conditional branch, no config read, and no carve-out.

2. Omni's ceilings exceed the clamp. omni/tool-result-media.ts carries MAX_UPLOAD_BYTES_PER_TOOL_RESULT = 128 MiB and omni/guard.ts carries DEFAULT_OMNI_MAX_UPLOAD_FILE_BYTES = 1 GiB. An image between 100 MB and those ceilings is exactly the class the funnel was built to deliver by reference.

3. The base did not bound at all. mcp-tool.ts at the merge base contains no bounding symbol; the part reached the funnel intact. The placeholder is new behaviour, not a documented limitation being made explicit.

Impact: In an omni-active session whose MCP tool returns an image over the source cap, the model now receives a text placeholder where it previously received the image by reference. The direction is fails-closed (nothing corrupted, model told media was omitted), but it is a functional regression on a supported configuration, and it removes precisely the large-media case reference delivery was built for.

Two ways to close this (consistent with qqqys's round-5 suggestion):

• Add a per-part carve-out: before the source clamp, check whether the part would be delivered by reference; if so, forward it untouched. The carve-out must be per-part (not per-config), because the funnel's decision is per-part and a config gate would reopen unbounded inline bytes on the wrong side.
• Keep the unconditional clamp and record the decision: add a comment at the clamp that names omni's higher ceilings and states that the trade is intentional, and update Risk & Scope accordingly. This is a documentation change, not a code change, and is the cheaper path.

What I verified as sound

One ceiling read. getMaxInlineMediaBytes() is read once into inlineByteCeiling at the top of boundInlineImageParts and used for both the renderer's adopt decision (passed to boundImageBuffer) and the trailing clampToInlineLimit. The two cannot disagree within a single result if settings change mid-loop. Correct.

loadSharp() extraction preserves error priority. prepareImage() now calls loadSharp() before any file-level check. An unavailable renderer reports renderer_unavailable rather than file_not_found — the constraint pinned by zoom-image.sharp-failure.test.ts. The same priority applies in prepareImageBuffer(): loadSharp() is called first, then the source-size check, then decoding. ✓

Shared predicate. The gate uses isImagePart(part) || mimeType === 'application/octet-stream', the same predicate the vision bridge and getMcpErrorImageContent use. This bound cannot drift from the repository's definition of "image", and MCP audio blocks (which carry an identical inlineData shape) stay excluded. ✓

Fail-open is scoped. A part the renderer could not decode is forwarded as received. The trailing clampToInlineLimit applies only when isImagePart(boundedPart) — an application/octet-stream blob that failed the renderer keeps its original bytes and no inline-limit placeholder is generated for a non-image. ✓

Envelope rewrite is safe. The update to the preceding text part fires only when boundedParts.at(-1) is a text part whose content ends with the exact string mime-type: <original>]. Absent that match, nothing is touched and no exception is possible. The mimeType value (from an untrusted MCP server) is used only in a string endsWith check, not a regex, so injection is not applicable. ✓

Error split is correct. renderer_unavailable is a persistent host condition (every image of every call fails the same way) → logged at warn. Individual decode failures are per-image → logged at debug. Unexpected non-ImageViewError exceptions are rethrown rather than swallowed. ✓

Placeholder wording overrides are complete and backwards-compatible. The MCP callers override limitLabel ("image source limit") and remedy ("Ask the user to have the tool return a smaller or lower-resolution payload.") — the shared default points the model at @file, which cannot exist for bytes that live only in an MCP response. The OversizedMediaPlaceholderOptions fields are optional and the defaults reproduce the previous text exactly, so no existing caller is broken. ✓

boundImageBuffer null contract. Returns null when the image already fits both the visual budget and the optional maxBytes ceiling, so small images keep their original bytes, format, and alpha channel. The caller checks if (view) before constructing a new part. ✓

Coverage gaps

The suite at this head is unit tests only. The omni integration path — an omni-active session, a trusted folder, a resolved upload channel, an MCP tool result with an over-100-MB image — is not exercised. That path is the one the blocker affects, and CI green at this head is consistent with the blocker being real.

Re-verified at 636d410. The head has not moved since my last review; the PR body has, and it now carries the part I asked for. Required CI is green at this head, with review-pr excluded as reviewer-bot infrastructure per my standing disclosure.

What changed, and it closes the concrete harm I named

Risk & Scope now states the withheld half rather than only the derivative half: a decoded image between the 100 MiB source cap and the per-result ceiling of the funnel is replaced by a text placeholder on the producer side, where base delivered it to the funnel for upload as a file reference. It names the band, names the base behaviour it replaces, links the tracking issue, and records that the both-clamps exemption is the eventual fix. In both languages. A reader merging this now learns the tradeoff from the change itself rather than only from a review thread, which was the specific harm I described, and that harm is closed.

What is still missing

The behaviour is unchanged, so the defect is present at this head, and I re-verified the four anchors rather than relying on the thread: the tool file contains no reference to omni at all, the pre-decode clamp runs unconditionally against the 100 MiB constant, the funnel per-result ceiling is 128 MiB, and the untouched precedent in the file-utilities path states that this cap is decoder protection and gates it on omni not taking the file. Three other reviewers have independently confirmed the same window at this head.

The acceptance recorded in the tracking issue is authored by the author of this PR, and it says so plainly: it rules as maintainer on his own change and invites another maintainer to say otherwise, in which case it will be revisited. The issue still carries a need-discussion label. Its triage record asks for a direction decision between exempting the clamps and recording the accepted loss, and describes the item as blocking for this PR rather than as a live user bug. My last review asked for a ruling from a maintainer who is not the author, and that specific element is still absent. An acceptance whose only effect is to remove the sole obstacle to a PR written by the acceptor is not a check on anything, however candid the wording is, and this wording is candid.

So the ask is narrow and cheap: one independent maintainer confirming the tradeoff in the tracking issue, or the both-clamps exemption. I am not contesting the trade itself. The band is narrow, the direction is fail-closed rather than a leak, and the argument that a delivery-mode branch is new surface in a PR whose scope is bounding is a reasonable one. I am recording that the acceptance is self-ruled and provisional by its own text, and as the pre-merge gate I cannot certify a Critical present at the reviewed head as resolved on that basis.

Review at head 75fcc4e (I am the author, so this is the review record for a non-author maintainer's vote — GitHub won't count mine).

The R3-1 / R4-1 opposing pair is closed at this head, in the shape R4-1's fix constraint demanded. I verified the mechanism directly rather than from the thread history:

1. Producer side is no longer omni-blind about the funnel, and no longer clamps ahead of it. boundInlineImageParts (mcp-tool.ts:1346) now takes omniDeliveryActive; under omni it forwards the renderer-bounded part unclamped, so a (100 MiB, 128 MiB] image reaches the funnel intact and can be uploaded as a fileData reference — R3-1's regression is gone.
2. The exemption is per-part, not per-config — R4-1's constraint. The funnel (omni/tool-result-media.ts) gained boundDeclinedInlineImage, applied only on the keep-inline exits. I walked every exit: sniff failure, modality disabled, count/byte budget exhausted, and transfer failure all route through keepInline → boundDeclinedInlineImage; uploaded parts never see it ("Uploaded parts never get here"); the guard-rejection path deliberately withholds rather than keeping inline (a policy verdict, not a transfer failure), with the resource handle preserved when one was recorded. So the clamp binds to the bytes actually staying inline — not to whether omni is active — which is exactly "bind the skip to the bytes actually being delivered."
3. The decoder-protection rationale is now coherent: the 100 MiB-era concern lives in boundImageBuffer (renderer), which runs in both modes; the inline clamp applies only where the bytes will actually be inlined.

Prior reviews on 636d410a (qqqys, chiga0, dev-bot) all confirmed the same three facts; this head resolves them structurally rather than by disclosure. CI on this head is green except the review-pr lane still running (bot infrastructure, not a gate), and all 32 threads are resolved.

Ask: one non-author maintainer vote to clear REVIEW_REQUIRED. Given the change shape now matches what qqqys and chiga0 each described as the correct fix, they're the natural reviewers to close this out.

Tier: Deep (cross-binary sharp dependency; omni delivery contract; new async image-processing path)

Verdict: APPROVE — R1-1 fixed; no new blockers at this head

Round-2 ledger

R1-1 — FIXED

The fix is split across two layers, each handling its half of the decision:

Producer layer (mcp-tool.ts): boundInlineImageParts now accepts omniDeliveryActive: boolean. When true, the function still applies the visual-budget resize (boundImageBuffer), but skips clampInlineMediaPart at the end — the part goes to the funnel at whatever size the renderer left it. The per-config check (isOmniMediaDeliveryActive()) gates on isOmniEnabled() first (fast path, no dynamic import for non-omni sessions) and then calls isOmniDeliveryActive(config).

Funnel layer (omni/tool-result-media.ts): Each of the four keep-inline exits now routes image parts through boundDeclinedInlineImage, which applies the same boundImageBuffer + clampInlineMediaPart combo as the non-omni producer path. So an image the funnel declines (failed sniff, disabled modality, budget exhausted, transfer failure) is bounded before reaching the model — the inline path has the same bounding whether it goes through omni or not.

The >100 MB path is covered in both cases:

• Non-omni: boundImageBuffer throws source_too_large (caught as ImageViewError), boundedPart remains the original, clampInlineMediaPart then withholds it as a text placeholder ✓
• Omni, uploaded by reference: boundImageBuffer throws source_too_large, boundedPart remains original, omniDeliveryActive ? boundedPart : ... returns original unmodified → funnel uploads by reference ✓
• Omni, funnel declines: boundDeclinedInlineImage also calls boundImageBuffer (same source_too_large throw, caught), then clampInlineMediaPart withholds as text placeholder ✓

Refactor changes at 75fcc4e6

Three things removed compared to my round-1 review head (636d410a):

1. Pre-decode 100 MB clamp removed. Previously boundInlineImageParts applied clampInlineMediaPart(part, IMAGE_MAX_SOURCE_BYTES) before calling boundImageBuffer. Now it just calls boundImageBuffer directly. For >100 MB images, boundImageBuffer → prepareImageBuffer → source_too_large throw (caught), and the trailing clampInlineMediaPart (in the non-omni branch) handles withholding. Identical outcome, one fewer gate.

2. application/octet-stream admission removed. Previously unlabelled blobs were admitted to the renderer for content sniffing. Now boundInlineImageParts gates strictly on isImagePart(part) — only declared-image-MIME parts are bounded. Unlabelled blobs pass through unchanged, restoring base behaviour. The commit message acknowledges this. Not a regression for the stated feature scope.

3. Envelope text rewrite removed. The logic that rewritten the preceding text envelope's mime annotation after a re-encode is gone. Simpler; no correctness impact since only labelled images are now processed and the envelope-rewrite was specific to the application/octet-stream sniff path.

What I verified as sound at this head

One ceiling read. getMaxInlineMediaBytes() is read once into inlineByteCeiling at the top of boundInlineImageParts and at the top of boundDeclinedInlineImage. The renderer's adopt decision and the trailing clamp cannot disagree within a single result. ✓

isImagePart gate. boundInlineImageParts in mcp-tool.ts gates on isImagePart(part), the same predicate the vision bridge uses. MCP audio blocks (identical inlineData shape) stay excluded. ✓

boundDeclinedInlineImage error handling. Non-ImageViewError exceptions rethrow; ImageViewError is caught and logged at debug. The trailing isImagePart(boundedPart) ? clampInlineMediaPart(...) : boundedPart correctly handles the case where the renderer fails: boundedPart remains the original image-typed part, gets clamped, becomes a text placeholder. ✓

isOmniMediaDeliveryActive() fast path. Non-omni sessions return immediately at isOmniEnabled() without loading the omni module. ✓

loadSharp() extraction preserves error priority. prepareImage() calls loadSharp() before any file-level check, so an unavailable renderer reports renderer_unavailable rather than file_not_found — the constraint pinned by zoom-image.sharp-failure.test.ts. Same in prepareImageBuffer. ✓

boundImageBuffer null contract. Returns null when the image already fits both visual budget and optional maxBytes ceiling, so small images keep their original bytes. The callers check if (view) before constructing a new part. ✓

TOOL_RESULT_MEDIA_REMEDY shared constant. Both mcp-tool.ts and tool-result-media.ts use the same exported constant for the recovery advice in text placeholders. No divergence possible. ✓

isImage signal at each keep-inline exit in tool-result-media.ts:

• Failed sniff: top === 'image' — declared MIME top-level is the best available signal when bytes couldn't be sniffed ✓
• Disabled modality, budget exhausted, transfer failure: sniffed.modality === 'image' — confirmed from actual bytes ✓

Reviewed with AI assistance.

Critical-only review at 75fcc4e6, base c502f3dc. Supersedes my comment at 636d410a.

Verdict: APPROVE — the blocking issue I filed, and which an independent deep review reached separately at the same head, is fixed. The fix takes the harder of the two paths I offered and in doing so also closes the per-config/per-part mismatch that an earlier round had raised. No Critical from my own scan of the three new commits.

Fixed: under omni delivery the tool no longer withholds what the funnel would upload

boundInlineImageParts now takes the delivery mode and stands the inline clamp down when omni is active:

boundedParts.push(
  omniDeliveryActive
    ? boundedPart
    : clampInlineMediaPart(boundedPart, inlineByteCeiling, TOOL_RESULT_MEDIA_REMEDY),
);

with omniDeliveryActive sourced from omni.isOmniDeliveryActive(this.cliConfig). So an image in the band between the decoder's source cap and omni's own ceilings reaches processToolResultOmniMedia intact and is uploaded by reference, which is what main did and what the funnel exists to do. The commit message states the both-or-neither reasoning correctly: exempting only the pre-decode clamp would just move the placeholder, because the renderer still refuses a source above the cap and the catch forwards the original into the trailing clamp.

Two tests pin this in opposite directions, which is what makes it a fix rather than a relaxation: delivers an over-limit image to omni instead of withholding it, and still withholds an over-limit image when omni delivery is inactive. A third, does not load the omni module when omni is disabled, pins that the exemption does not cost every non-omni session an import of the omni module.

Fixed, and at the right layer: the bound now follows the bytes actually delivered

Exempting on activation alone would have re-opened the earlier per-config/per-part finding, because the funnel's decision to take a part's bytes is per-part and it has several exits that keep the original inline. The second commit closes that by moving the bound to where the bytes are actually delivered. boundDeclinedInlineImage applies the same visual budget through boundImageBuffer and the same trailing inline clamp, and a keepInline helper routes every decline exit through it:

• if (!sniffed) return keepInline(part, bytes, top === 'image') — keyed on the declared top-level type, since sniffing failed and sniffed.modality does not exist;
• if (!modalities[sniffed.modality]) return keepInline(part, bytes, sniffed.modality === 'image');
• the exhausted-budget branch (uploadsRemaining <= 0 || bytes.length > uploadBytesRemaining), which is the 9th image of one result or the part that crosses the 128 MiB aggregate;
• the transfer-failure catch, whose contract is that one part's failure leaves that part inline rather than rejecting the whole tool result.

I checked the two early exits that still return [part] untouched, since an unbounded image leaving through either would defeat the change. Neither can carry one: the first requires !inline?.data || !inline.mimeType, so there are no bytes to bound; the second requires the declared mime's top level to be something other than image, audio or video, so a part declared image/png proceeds to sniffing rather than exiting. Transport-guard rejections remain withheld with a text placeholder and are never delivered inline, which is correct — delivering them would bypass the enabled guard.

The backstop for a declared image the renderer cannot decode is also right: boundImageBuffer throws, the ImageViewError is caught and logged, boundedPart stays the original, and the trailing isImagePart(boundedPart) ? clampInlineMediaPart(…) : boundedPart still applies the inline ceiling. An undecodable oversized image therefore becomes a placeholder rather than reaching the model at source resolution and being re-sent every turn. Non-image parts keep the verbatim pass-through, and keepInline sets the changed flag only when it actually replaced a part, so the caller cannot be told the result was rewritten when it was not.

Three decline-exit witnesses cover this side: bounds an over-budget image it keeps inline instead of delivering source-resolution bytes, bounds an image whose upload fails instead of delivering source-resolution bytes inline, and re-encodes a declined image that fits the visual budget but outweighs the inline ceiling. The tool side keeps its own set, including bounds an oversized image returned with an MCP tool error, so the error path is bounded too, and forwards an image the renderer cannot bound unchanged pins the fail-open.

The trim commit reduces scope back toward base without dropping a protection

The third commit removes three review-driven extras. Each returns to base behaviour rather than losing a guarantee, and I checked the reasoning rather than accepting it:

• The separate 100 MiB pre-decode clamp is gone, outcome-equivalently. The renderer already refuses a source above the cap with source_too_large; that is an ImageViewError, so the catch forwards the original part, and the trailing inline clamp — an order of magnitude below the source cap — then withholds it. The model sees the same placeholder, via one path instead of two.
• Untyped application/octet-stream resource blobs are no longer admitted to the renderer, and the matching funnel branch that base64-decoded every such blob to sniff it is gone. Bounding keys on the declared image mime, as it did at base. This narrows the change's reach, and the earlier round's concern about that gate firing unconditionally for newly admitted blobs no longer has a subject.
• OversizedMediaPlaceholderOptions collapses to a single optional remedy string. clampInlineMediaPart(part, limitBytes = getMaxInlineMediaBytes(), remedy?: string) keeps the parameter optional, so the four other callers — nonInteractiveCli.ts, tool-result-vision-bridge.ts, live-session.ts and use-llm-stream.ts — are untouched and keep the default @file-flavoured wording, which is still the correct advice for bytes that live on disk. TOOL_RESULT_MEDIA_REMEDY is shared by the MCP and omni sites so the two cannot drift. Lint & Static passing at this head is what confirms the signature change compiles across all of them.

One consequence of the trim, recorded and explicitly not filed: after a re-encode changes the mime, the envelope text part that precedes the media still announces the server's original mime, because the rewrite was removed. The label is informational — the model receives the actual bytes in the inline part, and the omni upload path carries its own mime on the fileData — so this is a stale string in context rather than a correctness, security or data-loss defect.

The error-handling split survives all three commits at both sites: only ImageViewError is caught, anything else rethrows, renderer_unavailable logs at warn on the tool side because it will fail every image of every call while single-image decode failures stay at debug, and the shared AbortSignal still reaches boundImageBuffer so an aborted request does not leave a resize running.

CI

12 checks pass at this head — including Lint & Static, Integration Tests (no-AK, No Sandbox), both Desktop Shell lanes and precheck-pr — with Test (ubuntu-latest, Node 22.x) and review-pr still in flight. I did not wait on them; the ubuntu test lane pending is not a gate here, and nothing is failing. No review thread is unresolved, and the decision state is back to REVIEW_REQUIRED.