feat(mcp-gateway): hold tool-result image blocks to the inline-image budget (#4010)

[bolichen97]

Summary

Images arriving as MCP tool-result content blocks bypass the prompt-path image downscale (acp/prompt_blocks.py) entirely: the gateway relay forwards them raw into kiro-cli's conversation history. Because kiro-cli replays the full history to the model every turn, a single image over the backend's per-image limits (2000 px longest edge for many-image requests, 5 MiB base64) permanently wedges the session — the offending block sits at a fixed history index and nothing can evict it (issue #4010's Bedrock error).

This PR applies the same, already-tested budget at the one tool-result seam Kiro Crew owns: the MCP gateway's response relay.

What changed

• kiro_crew/imaging.py (new) — the budget constants and Pillow machinery, relocated from acp/prompt_blocks.py (which re-exports the constants) and hardened in the move, plus the public downscale_image_block() entry point. A leaf module on purpose: the gateway daemon (gatewayd) imports it via mcp_gateway.backend, and importing it from acp.prompt_blocks would pull the whole ACP client into the broker and close an import cycle back into mcp_gateway. Pillow loads lazily on the first image. New MAX_IMAGE_SOURCE_PIXELS ceiling (64M px) refuses adversarially compressed small-bytes/huge-area rasters on the header read, before any decode — Pillow's own bomb guard only hard-errors at ~357M px (~1.4 GiB of pixels).
• mcp_gateway/image_budget.py (new) — rewrites every {"type":"image"} block in a relayed tools/call response: correctly-labeled, canonically-encoded compliant images ride through byte-identical (a compliant image with a lying mimeType or wrapped base64 keeps its bytes but gets the header-detected label / canonical encoding, since the byte ceiling is measured on the canonical form); oversized ones are downscaled in place; a block whose compliance cannot be established (undecodable, bomb refusal, over the source ceiling, still over the byte ceiling at the 256 px floor, or Pillow absent) is replaced by a text block — fail closed per block. Base64 decodes whitespace-tolerantly so base64.encodebytes-style 76-column producers are not regressed. Cumulative decode work per response is bounded by a source-pixel budget (MAX_FRAME_SOURCE_PIXELS, 4x the per-image ceiling, charged from header reads): blocks past it are replaced by a distinct "too much image processing" text block without ever being decoded, so a legitimate many-small-image result passes whole while an adversarial frame is bounded at four maximum-size decodes. Structural failures (non-JSON, no result.content) forward the original line, mirroring the sibling spill layer.
• mcp_gateway/backend.py — the stdout-pump hook, before the spill step. The pre-filter is escape-aware and its negative answer is provable: evading the literal "image" probe requires writing a letter as a JSON \u escape (the only escape yielding a letter), and the \u bytes are the probe's second half. Pillow work runs on a dedicated pool.
• executors.py — new bounded image_executor() (2 workers, mc-image), following the discovery_executor precedent: externally-paced, seconds-long Pillow work queues among itself and can never occupy the maintenance workers the orphan sweeps need.
• Docs — mcp-gateway-oversize-response.md gains a "Layer 3" section; acp-client.md points at the relocated machinery.
• Tests — test/test_mcp_gateway_image_budget.py (new, 24 tests): downscale-in-place, byte-identical pass-through, all five fail-closed paths, escaped-type rewrite, whitespace-wrapped base64, probe soundness, entry-point budget checks. Two pump-wiring tests in test_mcp_gateway_backend_coverage.py.

Declared behavior change on the prompt path

The relocated machinery is NOT byte-identical to what prompt_blocks.py had, and the prompt path (dashboard/Slack/Discord attachments) inherits the hardening: (1) a truncated-but-header-readable file that previously inlined byte-identical now fails closed to a path reference (img.verify() on the pass-through branch); (2) a source over 64M pixels now fails closed before any decode (MAX_IMAGE_SOURCE_PIXELS); (3) the emitted mimeType now tracks the header-detected format rather than the file extension, so a mislabeled attachment reaches the backend with the label its bytes actually are. All three close the same wedge class on the prompt path that this PR closes on the tool-result path; they are deliberate, tested, and called out here so they get reviewed as behavior changes, not riders.

Scope, disclosed honestly

The issue's primary reproducer — kiro-cli's own built-in Read tool in image mode — never transits Kiro Crew (Kiro Crew advertises fs.readTextFile: false; kiro-cli reads files and stores tool results itself). That path must be capped upstream in kiro-cli and is tracked there (kirodotdev/Kiro#9707). What Kiro Crew can and now does enforce is every brokered MCP server's tool result (browser/Playwright captures and any third-party server) when the MCP gateway is enabled — the interception point the issue asks for ("KiroCrew's own tools... will produce the same problem until KiroCrew enforces the cap"). MCP embedded-resource blobs are deliberately not rewritten (no brokered path renders them to the model today); documented next to the built-in-tools carve-out.

Pre-push review (dual model-pinned lanes)

• GPT lane: 2 High — probe escape-evasion (fixed: escape-aware provable pre-filter), unbounded decode cost (fixed: MAX_IMAGE_SOURCE_PIXELS header-read ceiling).
• Opus lane: 2 BLOCKING — acp import cycle into gatewayd (fixed: leaf imaging.py), Pillow work on the maintenance pool (fixed: dedicated image_executor). 4 advisory — strict base64 regression (fixed: whitespace-tolerant), fail-open at debug level (fixed: warning + server name, scheduling vs rewrite failures now distinguishable), embedded-resource scope (documented carve-out), no-Pillow fail-open (fixed: gateway seam fails closed).

Testing

• Full backend suite: 55251 passed; the 4 failures + 1 error are the known pre-existing env baseline, verified identical on pristine main in the same environment.
• isort / flake8 / mypy (981 files) clean; brand, harness-parity, docs-lint, scrub-lint gates green.
• Import isolation verified: import kiro_crew.mcp_gateway.backend loads zero kiro_crew.acp modules and no PIL; Pillow loads on the first image block.
• No frontend changes; no UI, so no screenshots.

Closes #4010

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7ba19a7c165cc8414228a333a3f0f6f052ec563c and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/imaging.py:146 -- function-local "from PIL import Image, ImageOps" violates top-level-imports -> Fix: move the guarded import to module scope.
[GPT-REVIEWED] 7ba19a7

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 7ba19a7c165cc8414228a333a3f0f6f052ec563c: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Advisory premise-level review of 7ba19a7c165cc8414228a333a3f0f6f052ec563c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

First-Principles-Verdict: PASS

A reported permanent-session-wedge (#4010) fixed at the deepest seam Kiro Crew owns, with every rider declared and traced to the same wedge class.

What this change ships

Intent: stop one oversized MCP tool-result image from permanently wedging a session (issue #4010) — a FIX.

1. Oversized images in brokered tool results are downscaled before entering history — justified (reported defect)
2. Unverifiable image blocks become explanatory text, fail-closed per block — justified (same wedge, derived)
3. Compliant images with wrapped base64 or lying mimeType get canonical encoding/correct label — justified (ceiling measured on canonical form)
4. Per-response decode budget; excess images dropped undecoded with a distinct note — justified (measured worker cost)
5. Omission notes capped at 4 plus counted summary — justified (amplification past the relay read limit); declared via the cited Layer 3 doc
6. Prompt path: truncated-but-readable attachments now fail closed to a path reference — rides along, declared, same wedge class
7. Prompt path: sources over 64M px refused before decode — rides along, declared, derived (Pillow's own guard errors only at ~357M px)
8. Prompt path: mimeType tracks header-detected bytes, not file extension — rides along, declared
9. New 2-worker mc-image pool — justified (externally-paced seconds-long CPU kept off the orphan-sweep workers)
10. Budget machinery moved to leaf kiro_crew.imaging, constants re-exported — justified (import cycle into gatewayd; 2 counted consumers of downscale_image_block: prompt_blocks.py:204, image_budget.py:185)

The true root cause (kiro-cli replaying unbounded history images) is out of scope and explicitly tracked upstream (kirodotdev/Kiro#9707); this sits at the deepest reachable mechanism. No existing mechanism does this job — the spill layer (mcp_gateway/spill.py) rewrites only type=="text" blocks (grepped: 0 image handling). No config keys, flags, or env vars added; the three declared prompt-path riders are the relocation's hardening, not scope creep.

[FIRST-PRINCIPLES-REVIEWED] 7ba19a7

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 7ba19a7c165cc8414228a333a3f0f6f052ec563c — updated in place on each push; does not block merge.

Design-Verdict: PASS

Right seam, right budget: reuses the tested prompt-path machinery at the one tool-result hop Kiro Crew owns, fail-closed where the wedge is, with honestly-scoped limits.

Suggestions

• "Closes Enhancement: apply image downscaling to tool-result image blocks, not just session/prompt attachments #4010" auto-closes an issue whose primary reproducer (kiro-cli's built-in Read, per your own scope note) stays open upstream — retitle/split the issue instead of closing, so wedge reports via that path don't land on a closed ticket.

[DESIGN-REVIEWED] 7ba19a7

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7ba19a7c165cc8414228a333a3f0f6f052ec563c — this comment is updated in place on each push.

Review details

Both candidates are self-rated low-confidence and describe the same class of concern: bounded, contained CPU/latency effects on purpose-built dedicated thread pools — not a crash, data loss, corruption, security hole, or removed guard.

Candidate 1 (per-frame decode budget under-counts shrink re-decodes): I confirmed _fit_encoded_budget re-decodes the original bytes each shrink pass (imaging.py:768–790, _downscale_within_limits(raw_bytes, …) inside the loop) and that _source_pixel_charge charges the source area only once (image_budget.py:1273). So the worst-case per-frame decode work does exceed the "four maximum-size decodes" the comment claims. But the effect is entirely confined to the dedicated 2-worker mc-image pool (executors.py:230–247), which exists precisely to absorb this externally-paced work; it cannot starve the maintenance/orphan-sweep workers. The observable outcome is thus at most elevated latency for co-pooled image work — a comment-accuracy/efficiency matter, which this pipeline does not report and which does not meet the BLOCKING classes.

Candidate 2 (large-frame JSON parse on the maintenance pool): I confirmed parse_image_bearing_frame runs on maintenance_executor() (backend.py:1069–1073). But the spill step immediately below (backend.py:1098–1106) already runs maybe_spill_response on that same pool for every large frame, so large-frame work on the maintenance pool is a pre-existing, established pattern, not one this diff introduces. The stated outcome ("can occupy… delaying… partially reintroducing starvation") stays in "could"/"can" territory and orphan sweeps tolerate delay — no clean (c) observable wrong outcome at the 80+ bar.

No new grounded finding emerged from reading the two new modules, the pump integration, or the executor pool.

No findings.

[OPUS-REVIEWED] 7ba19a7

Verdict parsed from the review's SHA-scoped output markers for commit 7ba19a7c165cc8414228a333a3f0f6f052ec563c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 7ba19a7c165cc8414228a333a3f0f6f052ec563c: <one-sentence reason>

[bolichen97]

Review round 1 dispositions (new head 4dbf4e69a):

GPT 5.6 BLOCKING — header-only pass-through admits truncated images: FIXED. The within-cap branch in kiro_crew/imaging.py now calls img.verify() before returning the original bytes — structural walk (checksums/markers), no pixel decode, raises on truncation and lands in the existing fail-closed handler. Regression tests added at both layers: test_truncated_image_with_readable_header (gateway rewrite → omission block) and test_truncated_image_returns_none (entry point). The finding was exactly right: a truncated file with a readable header was the one input that could ride the byte-identical fast path into replayed history as a corrupt block.

First Principles CONCERNS — prompt path skips the new source-pixel ceiling: SUBTRACTION TAKEN. build_prompt_blocks now calls downscale_image_block(...) instead of _fit_encoded_budget(...), so the decode-pixel ceiling (and the new integrity verify) covers channel-delivered images too, and the entry point has its second real consumer. The _fit_encoded_budget/_b64_len private re-exports are dropped from prompt_blocks (the one test using _b64_len now imports it from its owning module). On the "Closes #4010 while the primary reproducer is upstream" watch item: kept as-is deliberately — the issue itself scopes the KiroCrew-side ask to the paths KiroCrew controls and tracks the kiro-cli builtin path at kirodotdev/Kiro#9707; both the PR body and the issue comment state this so the closing maintainer decides with full information.

Full suite re-verified at the new head: 55253 passed, remaining failures are the documented pre-existing environment baseline.

[bolichen97]

Review round 2 disposition (new head 540c7191a):

GPT 5.6 BLOCKING — fallback re-encode keeps the source mime (TIFF bytes labeled after re-encode as PNG): FIXED, class-wide. Per this pipeline's second-finding rule, the whole mislabel class was audited rather than point-patched:

1. Re-encode branch (the reported case): the emitted mime now derives from the format actually WRITTEN via a new _FORMAT_MIME table — an oversized image/tiff block downscales to PNG bytes labeled image/png.
2. Pass-through branch (same class, unreported): a compliant image whose server-supplied mimeType disagrees with its bytes (PNG data labeled image/jpeg) previously transited with the lying label; it now emits the header-DETECTED format's mime, bytes untouched.

Tests: test_unknown_source_mime_relabels_to_the_written_format (TIFF→PNG), test_lying_mime_label_is_corrected_on_passthrough, test_within_cap_gif_keeps_its_gif_label (guards that the GIF→PNG swap stays re-encode-only). One existing prompt-path test updated to use format-faithful fixtures (test_every_supported_suffix_maps_to_its_mime previously wrote one PNG byte-string under all six extensions — the emitted mime now tracks the bytes the backend actually validates, not the file extension).

Full suite re-verified at the new head: 55257 passed, remaining failures are the documented pre-existing environment baseline.

[bolichen97]

Review round 3 dispositions (new head 25f072f06):

GPT 5.6 BLOCKING — per-frame decode work unbounded: FIXED. New MAX_IMAGE_BLOCKS_PER_RESULT = 8: blocks past the cap are replaced by a distinct "too many images" text block without being decoded, so a valid frame stuffed with maximum-cost images can no longer monopolize an image-pool worker for its duration. Regression test asserts the excess blocks never reach the decoder (counting stub) and that the first 8 are processed normally. Doc updated in the same commit.

GPT 5.6 FINDING (advisory) — function-local PIL import violates top-level-imports: REBUTTED, keeping as-is. This import is deliberately lazy and documented at the site and in the module docstring. The pre-push Opus lane BLOCKING finding on an earlier revision measured the cost of eager loading in this exact module: the gateway daemon (gatewayd, respawned by a supervisor) imports kiro_crew.imaging at start via mcp_gateway.backend, and a top-level try/except ImportError Pillow import re-introduces ~80ms + dozens of modules per daemon respawn for a dependency needed only once an image block actually arrives. The two lanes are in direct conflict here; the resolution is the documented lazy import, which the top-level-imports rule (blocking: false) explicitly accommodates for justified cases. If maintainers prefer the eager form, it is a two-line change with a known boot-cost tradeoff.

[bolichen97]

Review round 4 dispositions (new head cffc6d931; GPT ✅ and Opus ✅ both clean since 25f072f06):

Design CONCERNS 1 + First Principles Watch — "moved verbatim" is inaccurate and the prompt-path hardening is undeclared: ACCEPTED. The PR description now says "relocated … and hardened in the move" and carries a dedicated Declared behavior change on the prompt path section naming all three changes (truncated files fail closed, >64MP sources fail closed, emitted mimeType tracks detected bytes) so a human reviews them as behavior changes.

Design CONCERNS 2 — probe's \u half routes CJK text frames through the 2-worker image pool: FIXED. The hook is now two-stage: a cheap parse-confirm (parse_image_bearing_frame, JSON parse + structural scan, no Pillow) runs on the maintenance pool like the spill rewrite, and only frames with actual image blocks reach mc-image (rewrite_image_frame). New pump test proves a probe false positive never touches the image stage.

Design CONCERNS 3 — block-count cap truncates legitimate tiny-image batches: FIXED by redesign. MAX_IMAGE_BLOCKS_PER_RESULT is replaced with a source-PIXEL budget (MAX_FRAME_SOURCE_PIXELS = 4× the per-image ceiling), charged from header reads before any decode: ten small PDF-page renders spend almost nothing and pass whole (new test), while the adversarial case is bounded at four maximum-size decodes (counting-stub test proves excess blocks never reach the decoder).

First Principles Subtraction — zero-consumer max_source_pixels kwarg: TAKEN. Dropped; tests patch the module constant like the siblings.

Design Suggestion — "Closes #4010" vs the upstream residual: keeping Closes, rationale on record. The issue's own text scopes the KiroCrew-side ask to seams KiroCrew controls and already links the upstream tracker (kirodotdev/Kiro#9707) for the built-in Read path; the issue comment and PR body both restate this. If wedge reports recur from the upstream path, they reproduce with the gateway disabled and triage cleanly against the upstream tracker. A maintainer who prefers to keep #4010 open can strike the Closes line at merge.

Full suite re-verified at the new head: 55255+ passed, remaining failures are the documented pre-existing environment baseline.

[bolichen97]

Review round 5 dispositions (new head 223fad98d; Design flipped to ✅ PASS, GPT/Opus remain ✅):

First Principles Subtraction — zero-consumer enforce_image_budget composition: TAKEN. Deleted from the module; the test file now defines the two-stage composition locally with a docstring stating it mirrors the pump exactly, so every assertion runs against the production path shape.

Watch: per-frame pixel budget undeclared in the description: ACCEPTED. The image_budget.py bullet now names MAX_FRAME_SOURCE_PIXELS, the distinct excess message, and the many-small-images pass-through property.

Watch: "byte-identical" claim contradicted by the mislabel fix: ACCEPTED, claim tightened (not the code). Now reads: correctly-labeled compliant images ride through byte-identical; a compliant image whose mimeType lies keeps its bytes but gets the header-detected label.

Watch: "Closes #4010" half-fixed: standing disposition from round 4 (kept deliberately; maintainer can strike the line at merge).

[bolichen97]

Review round 8 disposition (new head c5324be42):

GPT 5.6 BLOCKING — wrapped base64 can bypass the encoded-size ceiling: FIXED. The ceiling is measured on the canonical encoding, so canonical is now what goes on the wire: a block passes through untouched only when its data already equals the exact canonical form; a compliant image with wrapped/whitespace base64 keeps its bytes but is re-emitted canonically. This closes the near-limit case where the canonical length passed the budget while the forwarded whitespace-inflated original (~1.3% larger) exceeded it on the backend. Test updated to assert canonicalization; module docstring, Layer 3 doc, and PR body claims synced in the same push.

Running tally for maintainers: 6 GPT blocking findings across 8 review rounds, every one a genuine defect in progressively deeper territory (truncation integrity → mime labeling → per-frame resource bounds → whitespace DoS → budget starvation → encoding canonicality). All fixed with regression tests; GPT, Opus, and Design have each been ✅/PASS on at least one prior head of this same design, so the architecture itself has been stable since round 4 — the churn is boundary-hardening of the new module, which is exactly what an EXTERNAL-CONTENT seam should get.

[bolichen97]

Review round 9 disposition (new head 7c2e4515d):

GPT 5.6 BLOCKING — mass rejected blocks amplify the frame past the relay limit: FIXED, with a different mechanism than suggested. The defect is real (280k empty image blocks × ~250-byte omission notes ≈ 70 MiB out of an 8 MiB input → stub drops the frame), but the suggested fix — charging rejected blocks against the pixel budget — would reintroduce the round-7 starvation defect: rejected blocks cost a header read, not a decode, and charging phantom pixels lets junk starve legitimate images again. The actual defect is OUTPUT amplification, so the fix bounds output: at most 4 explanatory omission notes per response (_MAX_OMISSION_NOTES), everything past them dropped and counted into one trailing summary block — output stays ≤ input + ~1.5 KiB regardless of block count, and the pixel budget keeps accounting only real decode work. Regression test: 500 empty image blocks → 4 notes + 1 summary naming the other 496, output under 2× input.

GPT 5.6 FINDING (advisory) — lazy PIL import: standing round-3 rebuttal, third occurrence, not re-litigating.

[bolichen97]

Review round 10 disposition (new head 7ba19a7c1):

GPT 5.6 BLOCKING — within-cap unsupported formats pass through unconverted: FIXED. The byte-identical fast path now requires BOTH conditions: dimensions within cap AND detected format in the known-good table. A within-cap TIFF (or any format outside the table) falls through to the existing re-encode machinery as a format CONVERSION at original dimensions — no resize — emerging as PNG with a matching label. Regression test: 300x200 TIFF converts to a 300x200 PNG. This closes the last cell of the 2x2 matrix (dimensions x format) whose other three cells earlier rounds covered.

GPT 5.6 FINDING (advisory) — lazy PIL import: standing round-3 rebuttal, fourth occurrence.

Process note for maintainers: this session's autonomous fix budget (10 iterations) is now exhausted. Ten review rounds produced 7 genuine GPT blocking findings, each fixed with a regression test, and GPT/Opus/Design/First-Principles have each PASSed at least one head of this design. If the next round produces another blocking finding, this session escalates to a human decision rather than continuing to iterate.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #4079 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #4079: CONTINUE_DEVELOPMENT. PR #4025 landed content-derived labelling and fail-closed verification for every Pillow-present path, which is the majority of what this PR's description claims as new. The no-Pillow branch, the max_edge<=0 escape hatch, the suffix-keyed re-encode format, and the documented sink contract are not covered. Not closable as completed. Files: src/kiro_crew/imaging.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.