feat(attachments): full attachment support — any file type, multiple per message (CHOO-1802) - #93
Merged
Merged
Conversation
added 3 commits
July 30, 2026 07:05
Attachments were effectively images-only and one file per message. Any file type, both directions: - Inbound: drop the `image/` guards in the Slack, Mattermost and Discord adapters so every file type is downloaded. Everything downstream (bridge_core, client_base, read_context, download_media) was already mimetype-agnostic. - Outbound: remove the `m.image` branch in bridge_core; `m.file` now takes the same native upload path instead of degrading to a "sent a file that isn't relayed yet" notice. - Widen the channel tool's extension→mimetype map so type fidelity survives the round trip. Multiple attachments per message. Matrix has no multi-attachment event — one media event carries one mxc, and MSC4274 / MSC2881 are unmerged — so a message with n files is sent as n events sharing a group marker (`com.switch.attachment_group`) and coalesced back at both edges: - bridge_core stamps the group inbound (a platform post hands us all its files at once, so the count is known up front) and buffers outbound groups to relay them as ONE platform post, via a new adapter `send_attachments` (Slack `file_uploads`, Mattermost `file_ids`). - agent_client buffers group parts and emits a single MessagePayload carrying all attachments. - `send_attachment` accepts `paths`, and the media endpoint accepts repeated `files` parts. An event with no marker is simply a group of one, so this is backward compatible with media already in a room. Fail loud, never fake: - Enforce the size cap inbound (it was only checked on the agent upload and bridge relay paths), plumbed to adapters from `agent_media_max_bytes`. - A file that is oversize or fails to download is carried as an `AttachmentFailure` and disclosed in the room, never dropped. - Multi-file sends validate every file before sending any, so a bad file fails the whole call rather than half-posting a message. - An incomplete group is flushed after a timeout with an explicit "n of m" notice rather than being held forever or dropped. - Non-image attachments reach the agent as `file_path`, and ones that could not be fetched as `failed_attachments`.
Update the agent-facing contract in SKILL.md: any file type in both directions, several files per message, the new file_path / failed_attachments notification fields, paths on send_attachment, and the all-or-nothing validation on a multi-file send. Bump the plugin version so installs pick it up.
Add coverage for parse_attachment_group, the inbound group stamping and
failure disclosure in bridge_core, and the agent_client coalescing
buffer. Writing it surfaced three real defects, fixed here:
- bool is a subclass of int, so a {"index": false} marker parsed as
index 0 instead of being rejected as malformed.
- The flush timer was re-armed on every arriving part, making the
timeout bound the gap between parts rather than the group; a batch
dribbling in just under the deadline could hold the buffer open
indefinitely. It is now armed once per group.
- A coalesced payload was anchored on whichever part arrived last rather
than on part 0, so message_id was unstable and replies could thread
off a non-canonical event. Both the completed and timed-out paths now
anchor on part 0.
Each fix has a regression test verified to fail against the prior code.
|
Thank you for your contribution! Before we can merge it, please read our Contributor License Agreement and sign it by posting the following comment on this pull request: I have read the CLA Document and I hereby sign the CLA switch-worker-louis-remote seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the gap described in CHOO-1802: attachments were effectively images-only, one at a time.
What was actually broken
Mapping the pipeline first paid off — most of the plumbing was already type-agnostic. The blocks were at the edges:
if not mimetype.startswith("image/"): continueguards (Slack, Mattermost, Discord) that dropped non-image files to a debug log — a silent drop, and a direct violation of the repo's fail-loud rule.if msgtype != "m.image"branch inbridge_corethat posted_sent a file that isn't relayed yet_instead of uploading. Theelsebranch below it already handled any mimetype.Changes
Any file type, both directions
m.imagespecial case.Multiple attachments per message
Matrix has no multi-attachment event — one media event carries one
mxc, and MSC4274 / MSC2881 are unmerged. So a message with n files is sent as n events sharing a group marker (com.switch.attachment_group={id, index, total}) and coalesced at both edges:bridge_corestamps the group inbound (a platform post hands us all its files at once, so the count is known up front — no guessing) and buffers outbound groups into one platform post via a new adaptersend_attachments(Slackfile_uploads, Mattermostfile_ids— both native).agent_clientbuffers parts and emits a single payload carrying all attachments.send_attachmentacceptspaths; the media endpoint accepts repeatedfilesparts.An event with no marker is a group of one, so this is backward compatible with media already in rooms.
Fail loud, never fake
AttachmentFailureand disclosed in the room, never dropped.file_path; unretrievable ones asfailed_attachments.Platform coverage — stated plainly
Testing
644 passed, 0 failed; ruff + mypy clean. (141 errors in a local run are the pre-existing Postgres-dependent store tests — no DB in that environment.)
New coverage:
parse_attachment_groupedge cases, inbound group stamping + failure disclosure, outbound batching, and agent-side coalescing (in-order, out-of-order, ungrouped, and incomplete-with-timeout, asserting no buffer/timer leak).Writing the tests surfaced three real defects in the first pass, fixed in
6548217— each with a regression test verified to fail against the prior code:boolis a subclass ofint, so{"index": false}parsed as index 0 instead of being rejected.message_idunstable. Both paths now anchor on part 0.Notes
alembic headsprints exactly one head (b3f36489c258).SKILL.mdupdated for the changed agent-facing contract; plugin version bumped 0.2.0 → 0.3.0 per repo policy.🤖 Generated with Claude Code