feat: preserve opaque inbound attachments - #3754
Conversation
github-actions
Bot
added
fork
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
github-actions
Bot
added
readiness: checking
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound root-cause fix in the right shared layer; the one risk left open is unbudgeted disk exposure through a lifecycle with a known leak. WatchPer-message opaque exposure is Suggestions
[DESIGN-REVIEWED] af79162 |
First Principles Review (Fable 5, fork) — ✅ PASSPremise-level review of All claims verified against the base tree. Everything substantive in this change checks out: the encoder's suffix-only typing ( First-Principles-Verdict: PASS Verify the default-on exposure a human should own: 10 × 50 MB per message to gateway disk, Slack still writing synchronously on the event loop. What this change shipsIntent: give the agent the actual bytes of video/zip/binary/SVG attachments as local files its tools can act on, instead of an "unsupported type" note — ADDITION. Inventory (8 items)
The zero option on item 1 leaves users sending files the agent is told about but cannot open, and the consumption path is already recorded in base ( [FIRST-PRINCIPLES-REVIEWED] af79162 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
github-actions
Bot
added
readiness: action required
Review dispositions for
|
github-actions
Bot
added
the
merge conflict
cc8ad8e to
987f9d1
Compare
github-actions
Bot
added
readiness: checking
|
Follow-up: that flake also blocks the AI-review round from startingWorth flagging because it makes the re-run load-bearing rather than cosmetic. On The Stage-2 fork review lanes gate on that conclusion: # .github/workflows/fork-gpt-review.yml
workflow_run:
workflows: ["CI"]
types: [completed]
...
if: >-
github.event.workflow_run.event == 'pull_request'
&& github.event.workflow_run.conclusion == 'success'So That leaves a re-run of run For completeness, the previous revision |
Ask: could a maintainer re-run CI on
|
987f9d1 to
71f432d
Compare
Rebased onto
|
github-actions
Bot
added
readiness: checking
github-actions
Bot
added
the
readiness: action required
GPT 5.6 on
|
…rodotdev#3769) `acp/prompt_blocks.py` chose an image's wire `mimeType` from its path suffix. The suffix is a claim made by whoever named the file -- an upload handler that kept a client-supplied name, a channel that renamed an attachment, a screenshot tool with its own convention -- and every producer feeding this sink therefore needed its own guard to keep that claim honest. Two concrete failures came out of it: - A renamed image shipped mislabelled. An image already inside the dimension cap rides through `_downscale_within_limits` byte-identical, so a JPEG named `.png` reached the model as JPEG bytes declared `image/png`. - A non-image named like one was inlined anyway. `notes.png` became an image block the backend cannot decode. That is not a one-turn error: kiro-cli replays the full message history every turn, so a rejected block sits at a fixed history index and wedges the session from then on -- the same consequence the dimension and encoded-size caps already fail closed to avoid. The suffix now decides only which paths are CANDIDATES (it is what the regex matched on, and re-checking it keeps a `.txt` from costing a stat). What the file IS is decided from its bytes: - `_sniff_media_type` prefers Pillow's reported `format`, which comes from a real decode and is header-only, so it costs no decompression. - A magic-byte signature table is the no-Pillow fallback. A hand-stripped install would otherwise degrade to trusting the suffix, which is the property this sink exists to remove. `RIFF` is matched together with its form field so a WAV does not read as a WEBP. - An undecodable or unsupported candidate is left as a path, not inlined. - A content/suffix MISMATCH is not a refusal: the image is real, so it still travels, labelled by its content. Producers rename attachments routinely, and dropping a good image to placate a filename trades a capability for nothing. The mismatch is logged. This also fixes the re-encode, not just the label: `_downscale_within_limits` picks its Pillow save format from the mime, so a wrong mime produced a wrongly encoded payload as well as a wrong declaration. Scoped to the sink. The issue's follow-on -- removing now-redundant producer-side suffix tables -- is deliberately not here: those are defense in depth, and removing them is the part that needs the broader producer compatibility coverage kirodotdev#3754 flagged. - `test_acp_prompt_blocks.py`: new `TestMediaTypeFromContent` (14 tests) covers content-over-suffix, the renamed-but-real image, the fail-closed non-image / SVG-behind-a-raster-name / truncated cases, the signature table agreeing with Pillow on every supported format and refusing a RIFF that is not WEBP, and the re-encode following content. 16 tests fail on main. - `test_every_supported_suffix_maps_to_its_mime` now writes REAL bytes of each format; it previously wrote a PNG behind every suffix, which is why it could pass while the mime was read off the filename. - Full ACP/image suites: 2132 passed.
…rodotdev#3769) `acp/prompt_blocks.py` chose an image's wire `mimeType` from its path suffix. The suffix is a claim made by whoever named the file -- an upload handler that kept a client-supplied name, a channel that renamed an attachment, a screenshot tool with its own convention -- and every producer feeding this sink therefore needed its own guard to keep that claim honest. Two concrete failures came out of it: - A renamed image shipped mislabelled. An image already inside the dimension cap rides through `_downscale_within_limits` byte-identical, so a JPEG named `.png` reached the model as JPEG bytes declared `image/png`. - A non-image named like one was inlined anyway. `notes.png` became an image block the backend cannot decode. That is not a one-turn error: kiro-cli replays the full message history every turn, so a rejected block sits at a fixed history index and wedges the session from then on -- the same consequence the dimension and encoded-size caps already fail closed to avoid. The suffix now decides only which paths are CANDIDATES (it is what the regex matched on, and re-checking it keeps a `.txt` from costing a stat). What the file IS is decided from its bytes: - `_sniff_media_type` prefers Pillow's reported `format`, which comes from a real decode and is header-only, so it costs no decompression. - A magic-byte signature table is the no-Pillow fallback. A hand-stripped install would otherwise degrade to trusting the suffix, which is the property this sink exists to remove. `RIFF` is matched together with its form field so a WAV does not read as a WEBP. - An undecodable or unsupported candidate is left as a path, not inlined. - A content/suffix MISMATCH is not a refusal: the image is real, so it still travels, labelled by its content. Producers rename attachments routinely, and dropping a good image to placate a filename trades a capability for nothing. The mismatch is logged. This also fixes the re-encode, not just the label: `_downscale_within_limits` picks its Pillow save format from the mime, so a wrong mime produced a wrongly encoded payload as well as a wrong declaration. Scoped to the sink. The issue's follow-on -- removing now-redundant producer-side suffix tables -- is deliberately not here: those are defense in depth, and removing them is the part that needs the broader producer compatibility coverage kirodotdev#3754 flagged. - `test_acp_prompt_blocks.py`: new `TestMediaTypeFromContent` (14 tests) covers content-over-suffix, the renamed-but-real image, the fail-closed non-image / SVG-behind-a-raster-name / truncated cases, the signature table agreeing with Pillow on every supported format and refusing a RIFF that is not WEBP, and the re-encode following content. 16 tests fail on main. - `test_every_supported_suffix_maps_to_its_mime` now writes REAL bytes of each format; it previously wrote a PNG behind every suffix, which is why it could pass while the mime was read off the filename. - Full ACP/image suites: 2132 passed.
|
🤖 Kiro Crew [operator: bolichen97]: This PR was flagged for drive-to-green triage. I assessed the blockers, and they require a human decision rather than a mechanical fix. Current state on Why this is not auto-resolvable: GPT 5.6's blocking fix is "Restore VIDEO/OTHER rejection until Slack file writes are offloaded." The only two ways to satisfy that verdict are:
You (and a second model, Opus 4.8) have rebutted the finding as pre-existing: this PR only raises the accepted ceiling on that path from 20 MiB to 50 MiB, it does not introduce the sync write. Decision needed from a maintainer:
I have not merged, approved, or pushed anything. Once a maintainer resolves the GPT lane (override or scope guidance), the pipeline will re-assess on its next cycle. Add |
bolichen97
added
needs-author-decision
github-actions
Bot
added
the
merge conflict
…rodotdev#3769) `acp/prompt_blocks.py` chose an image's wire `mimeType` from its path suffix. The suffix is a claim made by whoever named the file -- an upload handler that kept a client-supplied name, a channel that renamed an attachment, a screenshot tool with its own convention -- and every producer feeding this sink therefore needed its own guard to keep that claim honest. Two concrete failures came out of it: - A renamed image shipped mislabelled. An image already inside the dimension cap rides through `_downscale_within_limits` byte-identical, so a JPEG named `.png` reached the model as JPEG bytes declared `image/png`. - A non-image named like one was inlined anyway. `notes.png` became an image block the backend cannot decode. That is not a one-turn error: kiro-cli replays the full message history every turn, so a rejected block sits at a fixed history index and wedges the session from then on -- the same consequence the dimension and encoded-size caps already fail closed to avoid. The suffix now decides only which paths are CANDIDATES (it is what the regex matched on, and re-checking it keeps a `.txt` from costing a stat). What the file IS is decided from its bytes: - `_sniff_media_type` prefers Pillow's reported `format`, which comes from a real decode and is header-only, so it costs no decompression. - A magic-byte signature table is the no-Pillow fallback. A hand-stripped install would otherwise degrade to trusting the suffix, which is the property this sink exists to remove. `RIFF` is matched together with its form field so a WAV does not read as a WEBP. - An undecodable or unsupported candidate is left as a path, not inlined. - A content/suffix MISMATCH is not a refusal: the image is real, so it still travels, labelled by its content. Producers rename attachments routinely, and dropping a good image to placate a filename trades a capability for nothing. The mismatch is logged. This also fixes the re-encode, not just the label: `_downscale_within_limits` picks its Pillow save format from the mime, so a wrong mime produced a wrongly encoded payload as well as a wrong declaration. Scoped to the sink. The issue's follow-on -- removing now-redundant producer-side suffix tables -- is deliberately not here: those are defense in depth, and removing them is the part that needs the broader producer compatibility coverage kirodotdev#3754 flagged. - `test_acp_prompt_blocks.py`: new `TestMediaTypeFromContent` (14 tests) covers content-over-suffix, the renamed-but-real image, the fail-closed non-image / SVG-behind-a-raster-name / truncated cases, the signature table agreeing with Pillow on every supported format and refusing a RIFF that is not WEBP, and the re-encode following content. 16 tests fail on main. - `test_every_supported_suffix_maps_to_its_mime` now writes REAL bytes of each format; it previously wrote a PNG behind every suffix, which is why it could pass while the mime was read off the filename. - Full ACP/image suites: 2132 passed.
…rodotdev#3769) `acp/prompt_blocks.py` chose an image's wire `mimeType` from its path suffix. The suffix is a claim made by whoever named the file -- an upload handler that kept a client-supplied name, a channel that renamed an attachment, a screenshot tool with its own convention -- and every producer feeding this sink therefore needed its own guard to keep that claim honest. Two concrete failures came out of it: - A renamed image shipped mislabelled. An image already inside the dimension cap rides through `_downscale_within_limits` byte-identical, so a JPEG named `.png` reached the model as JPEG bytes declared `image/png`. - A non-image named like one was inlined anyway. `notes.png` became an image block the backend cannot decode. That is not a one-turn error: kiro-cli replays the full message history every turn, so a rejected block sits at a fixed history index and wedges the session from then on -- the same consequence the dimension and encoded-size caps already fail closed to avoid. The suffix now decides only which paths are CANDIDATES (it is what the regex matched on, and re-checking it keeps a `.txt` from costing a stat). What the file IS is decided from its bytes: - `_sniff_media_type` prefers Pillow's reported `format`, which comes from a real decode and is header-only, so it costs no decompression. - A magic-byte signature table is the no-Pillow fallback. A hand-stripped install would otherwise degrade to trusting the suffix, which is the property this sink exists to remove. `RIFF` is matched together with its form field so a WAV does not read as a WEBP. - An undecodable or unsupported candidate is left as a path, not inlined. - A content/suffix MISMATCH is not a refusal: the image is real, so it still travels, labelled by its content. Producers rename attachments routinely, and dropping a good image to placate a filename trades a capability for nothing. The mismatch is logged. This also fixes the re-encode, not just the label: `_downscale_within_limits` picks its Pillow save format from the mime, so a wrong mime produced a wrongly encoded payload as well as a wrong declaration. Scoped to the sink. The issue's follow-on -- removing now-redundant producer-side suffix tables -- is deliberately not here: those are defense in depth, and removing them is the part that needs the broader producer compatibility coverage kirodotdev#3754 flagged. - `test_acp_prompt_blocks.py`: new `TestMediaTypeFromContent` (14 tests) covers content-over-suffix, the renamed-but-real image, the fail-closed non-image / SVG-behind-a-raster-name / truncated cases, the signature table agreeing with Pillow on every supported format and refusing a RIFF that is not WEBP, and the re-encode following content. 16 tests fail on main. - `test_every_supported_suffix_maps_to_its_mime` now writes REAL bytes of each format; it previously wrote a PNG behind every suffix, which is why it could pass while the mime was read off the filename. - Full ACP/image suites: 2132 passed.
…rodotdev#3769) `acp/prompt_blocks.py` chose an image's wire `mimeType` from its path suffix. The suffix is a claim made by whoever named the file -- an upload handler that kept a client-supplied name, a channel that renamed an attachment, a screenshot tool with its own convention -- and every producer feeding this sink therefore needed its own guard to keep that claim honest. Two concrete failures came out of it: - A renamed image shipped mislabelled. An image already inside the dimension cap rides through `_downscale_within_limits` byte-identical, so a JPEG named `.png` reached the model as JPEG bytes declared `image/png`. - A non-image named like one was inlined anyway. `notes.png` became an image block the backend cannot decode. That is not a one-turn error: kiro-cli replays the full message history every turn, so a rejected block sits at a fixed history index and wedges the session from then on -- the same consequence the dimension and encoded-size caps already fail closed to avoid. The suffix now decides only which paths are CANDIDATES (it is what the regex matched on, and re-checking it keeps a `.txt` from costing a stat). What the file IS is decided from its bytes: - `_sniff_media_type` prefers Pillow's reported `format`, which comes from a real decode and is header-only, so it costs no decompression. - A magic-byte signature table is the no-Pillow fallback. A hand-stripped install would otherwise degrade to trusting the suffix, which is the property this sink exists to remove. `RIFF` is matched together with its form field so a WAV does not read as a WEBP. - An undecodable or unsupported candidate is left as a path, not inlined. - A content/suffix MISMATCH is not a refusal: the image is real, so it still travels, labelled by its content. Producers rename attachments routinely, and dropping a good image to placate a filename trades a capability for nothing. The mismatch is logged. This also fixes the re-encode, not just the label: `_downscale_within_limits` picks its Pillow save format from the mime, so a wrong mime produced a wrongly encoded payload as well as a wrong declaration. Scoped to the sink. The issue's follow-on -- removing now-redundant producer-side suffix tables -- is deliberately not here: those are defense in depth, and removing them is the part that needs the broader producer compatibility coverage kirodotdev#3754 flagged. - `test_acp_prompt_blocks.py`: new `TestMediaTypeFromContent` (14 tests) covers content-over-suffix, the renamed-but-real image, the fail-closed non-image / SVG-behind-a-raster-name / truncated cases, the signature table agreeing with Pillow on every supported format and refusing a RIFF that is not WEBP, and the re-encode following content. 16 tests fail on main. - `test_every_supported_suffix_maps_to_its_mime` now writes REAL bytes of each format; it previously wrote a PNG behind every suffix, which is why it could pass while the mime was read off the filename. - Full ACP/image suites: 2132 passed.
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Video and unrecognized formats were rejected before download, so an inbound file the agent could have acted on arrived as a note instead of bytes. Keep them as byte-identical temporary files under a 50 MB cap, hand the agent the local path plus the original name, type and size, and reuse the existing per-turn cleanup ownership. Opaque bytes are never parsed or executed automatically. A sender picks filename and mimetype independently, so an opaque file could arrive named "photo.png" while declaring application/octet-stream. The ACP encoder types a prompt path by suffix alone, so such a path would reach the image sink without the content-signature check the IMAGE branch enforces -- emitted as image/png on the strength of a sender-supplied name. An inlineable image suffix is therefore stripped from the temporary path before ownership transfers, mirroring the retype the IMAGE branch already performs, and a rename that fails drops the attachment rather than emitting the original path. A contract test pins the suffix set against the encoder's own table.
71f432d to
af79162
Compare
|
Rebased onto main Conflicts:
Gates: isort and flake8 clean. Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong. |
github-actions
Bot
added
readiness: checking
github-actions
Bot
removed
the
readiness: passed
3 participants
Problem / Motivation
An inbound attachment in a format the gateway does not recognise never reaches the
agent as a file.
messaging/attachments.pyclassifiedvideo/*and everythingunrecognised as
VIDEO/OTHERand rejected both before downloading, so theturn carried only a note:
The bytes were available — Slack exposes them via
url_private_download, Discord andTelegram via their own CDNs — but the agent got a sentence about a file it could not
open. Sending a zip, an MP4, a binary payload, or an SVG produced a session that knew
something arrived and could do nothing with it.
Why it matters
The formats that are "unsupported" for model inlining are often exactly the ones a
tool-using agent is best at: unpack the archive, read the manifest inside it, hash the
binary, run a converter. Rejecting them before download decides on the model's behalf
that a file is useless, when the useful path was never the model's eyes — it was the
agent's file tools. Every channel inherited the same dead end, because the rejection
lives in the shared ingestion layer rather than in any one adapter.
What changed (motivation → approach → change)
Symptom → root cause. The rejection sat before
_fetch(), so no policy changedownstream could recover the bytes. The classifier's job — "can the model inline
this?" — had been overloaded into "should we keep this at all?". Those are different
questions: an MP4 is not inlineable, but it is perfectly storable.
Change.
VIDEOandOTHERnow flow through the same bounded download path theother classes use, and land in a new
IngestResult.file_paths:IngestLimits.max_opaque_bytes(50 MB, matching the Dashboard upload ceiling),checked against channel metadata before download and against the actual bytes
after, like every other class
tempfile.mkstemppath; nothingparses, extracts, or executes it
declared type and its real size
temp_pathsnow includesfile_paths, so the existing per-turn cleanup owns them —no new lifecycle
Weixin all get it
Slack's adapter returns
image_paths + file_pathsand keeps the historical(paths, text_blocks)shape; its queue kwarg keeps the nameimage_temp_pathsso noexisting caller or test has to move.
One hole found in review, and closed
A sender supplies filename and mimetype independently (Telegram
sendDocument,Discord
filename/content_type), so an opaque file can arrive namedphoto.pngwhile declaring
application/octet-stream.safe_suffixis traversal-safe but keepsthe extension, so the temp path ended
.png— andacp/prompt_blocks.pyselectsprompt paths and their
mimeTypeby suffix alone, with no comparison againstcontent. That path would be emitted as
image/pngon the strength of a sender-suppliedname, without the
sniff_image_mimecheck theIMAGEbranch enforces for exactly thisreason.
Scope, stated precisely:
_fit_encoded_budgetdoes call PillowImage.open, soarbitrary non-image bytes normally fail closed there. The residual exposure is a
decodable raster at a mismatched suffix travelling with untruthful wire metadata,
plus the
_HAS_PIL == Falsefallback which passes bytes through unparsed.Closed by stripping an inlineable image suffix before ownership transfers, reusing the
os.replaceretype theIMAGEbranch already performs — and failing closed: arename that does not happen drops the attachment (audited, with a visible reason)
instead of emitting the original path. A contract test pins the suffix set against
IMAGE_MEDIA_TYPESso the two cannot drift.Tests
Added to the existing files — no new test file:
block carries name, declared type and actual size
max_opaque_bytesenforced on downloaded bytes when metadata lies (size=0)temp_pathscovers image + audio + opaque, and cleanup removes all threevideo/webmvoice memo still transcribes under the audio override, while othervideo types are preserved as opaque files
png,PNG,jpg,jpeg,gif,webp,bmp), asserting.binas a hard-codedexpectation rather than against the production constant
acp.prompt_blocks.IMAGE_MEDIA_TYPESIngestResult's new field is appended last, pinned by a positional-construction testopaque file instead of rejected
file_paths, then caller cleanup[Attached file: archive.bin]and one.binpath, and the path is gone after the turnEvery new assertion was mutation-verified: with the guard removed each one fails. That
caught a real defect in the first draft of this PR's own tests — the suffix assertion
was written against
_INLINEABLE_IMAGE_SUFFIXESitself and passed vacuously when thatset was emptied.
Manual verification
N/A — covered by unit tests plus the mutation pass above. Not exercised: delivery
through a live Slack/Discord/Telegram workspace, which needs credentials and a real
inbound file.
Screenshots
N/A — backend only, no UI surface changes.
Pre-existing issues this change makes larger
Local review raised two more defects. Both are real and both predate this change. One has
since been fixed upstream and this branch is rebased onto that fix; the other is only
widened here and is disclosed rather than folded in.
RealSlackClient.download_filedoesopen(dest, "wb")andf.write(chunk)on the event-loop thread (8 KB at a time,with a network await between chunks). That file is untouched here; the same path
already served
IMAGE,TEXTandDOCUMENT. What changes is the largest acceptedpayload on it: 20 MiB → 50 MiB.
reported as fix: unlink queued attachment temp files when entries are discarded #3768 and fixed by fix: unlink temp files when queued entries are discarded (#3768) #3776 (
717b0c91), which this branch is rebased onto.session.unlink_queued_temp_paths()unlinks every path in a discarded entry'simage_temp_paths, oncancel_queued,clear_queue,dequeue's cancelled-skip, andthe
_pending_queuedrops in_handle_message_deletedand the!stophandler.Because this PR routes image, audio and opaque paths through that same kwarg, opaque
files are covered by that fix without further change here. The remaining known gap is
upstream's own: session-teardown paths (restart/remove/destroy/idle sweep) drop
session.queuewithout unlinking.Two further notes worth your judgement: retained opaque bytes are bounded only by
max_attachments × max_opaque_byteswith no aggregate budget, and Weixin'sfetch_cdn_bytesbuffers a whole object in RAM before writing, which this change makesnewly reachable for video and unrecognised items (bounded by its own 32 MB
MAX_CDN_BYTES).Notes
docs/system-specs/modules/messaging.mdandslack-gateway.mdare updated in thiscommit, per AGENTS.md. Those are the only two module specs that document inbound
attachments.
isort,flake8,mypy src/kiro_crew/, plus thefrontend
tsc -b+ build, vitest (18128 passed) and the Electron suite (922 passed).mypyreports 3 pre-existingos.*xattrerrors inhooks.pythat only appear onmacOS.
Closes #3515