feat(telegram): inbound attachment support (image vision, documents, audio transcription)

[chenmingwei23]

Scope note: #2213 (album coalescing) was merged into this branch and then squashed in, since the repo enforces one commit per PR. This PR is now the single change that lands both on main. Closes #2200 and #2205.

Problem

Telegram users sending photos, documents, or voice memos got no response: transport.receive() returned early on if not inbound.text, so every non-text message was dropped silently, with no explanation.

Worse for the primary use case — Telegram does not deliver an album as one message. Selecting four screenshots and sending them with one question produces four separate message updates sharing a media_group_id, with the caption on only one member:

update	content	what the agent saw
1	photo1 + caption	image + the question
2	photo2	a bare image, no context
3	photo3	a bare image, no context
4	photo4	a bare image, no context

Why it matters

Screenshots are the reason to send an image to an assistant at all. Discord already supported inbound attachments; Telegram users were locked out, and multi-image — the ordinary case — was structurally broken rather than merely unsupported.

Fix (symptoms → root cause → change)

Root cause was two layers: nothing extracted file metadata from Telegram updates, and nothing ever looked at media_group_id (zero references before this change), so the envelope's one-album-is-N-updates shape reached the shared pipeline unmodified.

Generalized into the shared layer — so wording and handling cannot drift between channels:

• append_attachment_context() hoisted out of discord/attachments.py into messaging/attachments.py.
• transcribe_audio_attachments() hoisted likewise. Both channel adapters carried byte-identical STT-availability + transcription loops (flagged by Design Review); each now ends in one delegating call, so the transcript wording, the STT-unavailable rejection text, and the off-loop availability check exist in exactly one place.

Telegram-specific:

• telegram/attachments.py maps photo/document/audio/voice/video onto the shared Attachment shape.
• TelegramClient extracts photo[-1] (largest), document, audio, voice, video, animation; uses caption as the text fallback; downloads via getFile with host allowlisting, token-free error messages (the download URL embeds the bot token, so aiohttp's default exception text would leak it into gateway.log), and file I/O offloaded to asyncio.to_thread so a 20 MB document cannot stall the gateway event loop.
• TelegramTransport: files_inbound=True, accepts attachment-only messages.
• TelegramDispatcher ingests after session acquire and propagates attachments through the mid-turn queue and drain.

Album coalescing — envelope layer only, deliberately not generalized. Discord's MESSAGE_CREATE carries the whole attachments[] array and Slack's message event carries files[], so the shared layer's "one message, N attachments" contract already holds for them, and WeCom ingests no files. Telegram is the only channel needing its envelope restored to that shape, and it must happen before entering the shared pipeline; hoisting it would push a transport quirk into the layer whose whole value is being channel-neutral.

• _dispatch buffers members keyed by (chat_id, media_group_id). Keying on the id alone would merge two chats' members into one message addressed to head.chat_id — leaking content across a conversation boundary and swallowing the second chat's copy.
• The merge concatenates attachments in member order, takes the caption from the first non-empty member (not assumed to be first), and keeps the head message_id so a reply or steer-ack targets the album's first message.
• Bounded on every axis: per-member rearming window (flush follows the last arrival), hard ceiling from the first member (an endless stream cannot defer the flush forever), per-group member cap (logged when hit, never silently truncated), concurrent-group cap that force-flushes the oldest rather than dropping it, and a best-effort flush on close().

Channel parity gap restored. discord/transport_dispatch.py gates steering on and not msg.attachments; Telegram never had it. steer forwards text only, so an attachment message arriving mid-turn was steered caption-only and silently lost every file. Album buffering made this routinely reachable — a follow-up typed during the debounce window starts a turn, so the album's own flush lands mid-turn. Telegram now gates identically, and such a message always takes the queue path, which carries attachments.

Known residual — tracked in #2217

An inbound message arriving inside the shutdown window is refused by SessionManager._closing and lost. This is pre-existing and channel-agnostic: shutdown runs channel teardown and close_all() concurrently, so a plain single message in that window is refused identically today with no attachments involved. Buffering widens it by at most the debounce interval. The proposed "flush before close_all()" reorder does not fix it — close() never awaits handler tasks, so delivering would require awaiting an unbounded agent turn against the existing timeout=2.0. The real fix is durable inbound persistence (#2217). Documented in code and pinned by test rather than claimed away.

Tests

test/test_telegram_attachments.py (18) — dispatch extraction, _to_attachment normalization, ingest end-to-end (image download, document text extraction, video rejection, voice transcription with and without STT), capability flags.

test/test_telegram_album.py (11) — 4→1 turn with order and caption preserved; caption recovered from a later member; head message_id; non-album message unaffected; interleaved groups separate; same group id in two chats does not merge; buffer fully drained; member cap logged; group cap force-flushes; hard ceiling flushes a never-ending group; close() attempts delivery.

test/test_messaging_attachments.py (+7) — the hoisted append_attachment_context.

test/test_telegram.py (+1) — an attachment message is queued, not steered, and the attachments survive the queue (the weaker "steer was skipped" assertion would pass even if the queue dropped them).

All guards are mutation-verified: removing each makes a specific named assertion fail. Two tests were tautological on the first pass and were fixed — the close() test passed with the flush deleted because the fast test window let the natural timer fire, and a contract test asserted on module source where the import line alone satisfied it.

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @chenmingwei23 overrides the GPT 5.6 finding for b552d6acbec81b77112e351f1e366a20cd411959; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Right layers throughout: envelope quirks (albums) stay in the client, channel-neutral logic hoisted to messaging/attachments.py, bounded buffering with visible-not-silent overflow.

Suggestions

• The drain/collapse loop with attachment-cap deferral, the steer-path attachment gate, and the ingestion+cleanup block are now a third hand-maintained near-copy across Slack/Discord/Telegram dispatchers ("Mirrors discord/transport_dispatch.py" appears five times) — and this PR itself proves the drift mode: Telegram was missing two gates Discord had, silently dropping files. Hoist the queue-drain/steer-gating skeleton into the shared messaging layer as a follow-up so the fourth channel can't repeat it.

[DESIGN-REVIEWED] b552d6a

[github-actions]

Opus 5 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] b552d6a

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

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

[chenmingwei23]

Dispositions for findings raised on 8dfa00e54 (now superseded by a266c1f1ddced767aa1e49fa7b8c53ddd4469fb1).

GPT 5.6 / Opus 5 — BLOCKING, telegram/client.py:523 — failed downloads expose the bot token — FIXED
Both reviewers were right. resp.raise_for_status() raises ClientResponseError, whose str() embeds real_url = https://api.telegram.org/file/bot<TOKEN>/…, and ingest_attachments logs that via logger.exception, so the token would land in gateway.log. Replaced with a token-free ValueError(f"Telegram file download failed (status {resp.status})"), and wrapped the request in an except (aiohttp.ClientError, asyncio.TimeoutError) that re-raises as ValueError(f"… transport error ({type(exc).__name__})") with from None — so neither the status path nor the transport path can carry the URL into a log. This matches the existing convention in this file (client.py already logs only type(exc).__name__ in its other two aiohttp handlers for exactly this reason).

GPT 5.6 / Opus 5 — BLOCKING, telegram/client.py:524 — attachment writes block the event loop — FIXED
Correct, and it violates no-blocking-call-on-event-loop. open() plus every write() ran inline on the gateway loop, so a 20 MB document on network/FUSE-backed TMPDIR would stall every other session and the liveness heartbeat. Now mirrors discord/client.py: fh = await asyncio.to_thread(open, dest, "wb"), each await asyncio.to_thread(fh.write, chunk), closed in a finally via asyncio.to_thread.

Design Review (advisory 🟡, non-blocking) — process_telegram_attachments duplicates process_discord_attachments — FIXED, not deferred
This was the right call and I took it rather than accepting the duplication. Hoisted the whole STT-availability + transcription loop into messaging/attachments.py as transcribe_audio_attachments(result, source), parameterized on source for log lines only. Both adapters now end with a single return await transcribe_audio_attachments(result, "…"), so the transcript wording, the STT-unavailable rejection text, and the off-loop availability check exist in exactly one place. kiro_crew.transcribe is imported lazily inside the helper so this shared core module does not pull the optional voice extra at import time. Each adapter is now purely envelope-mapping + download-auth, which is the only genuinely channel-shaped part.

Guarded by a source-level contract test (test_transcription_block_is_shared_not_duplicated) asserting each adapter's function body calls the helper and that the duplicated block's tell-tale strings are gone from the module. I mutation-tested it: my first version asserted on the module source and passed with the call site deleted (the import line alone satisfied it) — tautological. Scoped to inspect.getsource(fn), and re-verified the mutant now fails with the specific named assertion for both adapters.

Automated Rule Check — Use lucide-react icons instead of inline SVGs — NOT MINE, resolved by rebase
This PR touches eight files, all .py; zero .tsx/.ts. The check diffs BASE..HEAD where BASE was main tip 906557d1e, which had advanced past my merge-base 89bf7982c. Main had replaced those inline SVGs with lucide-react icons, so a two-dot diff from newer-main to my stale branch reported main's own removals as my additions, at website/ line numbers this branch never touched. Rebased onto c831e58d0; merge-base now equals main tip, so the phantom diff is gone.

Backend Lint & Type Check — isort — FIXED
Real failure, mine. The # noqa: F401 I put on the re-export import line made isort split it into two imports from the same module, and isort was not idempotent about re-merging them, so local and CI disagreed at the same pinned 6.0.0. Removed the noqa entirely in favour of an __all__ declaration, which satisfies pyflakes F401 for a re-export without a blanket noqa that would also have suppressed genuine unused-import warnings for Attachment/IngestResult/ingest_attachments on the same statement. Verified isort --check-only src/kiro_crew test is clean across two consecutive runs, plus flake8 and mypy src/kiro_crew/ (847 files, CI-parity venv: mypy 1.14.1 matching the pin, no faiss).

Note on a pre-existing local failure (not introduced here, green in CI)
test_telegram.py::TestTelegramMidTurn::test_concurrent_queue_adds_share_one_receipt fails intermittently in my sandbox. I verified it against clean main in a separate worktree at c831e58d0 with PYTHONPATH pointed at that tree — it fails identically there with none of this branch's code loaded, and CI's Backend Tests shards pass it. Pre-existing and unrelated; not touched.

[chenmingwei23]

Design Review (advisory 🟡 suggestion on a266c1f1ddced767aa1e49fa7b8c53ddd4469fb1) — multi-photo albums (media_group_id) arrive as N separate turns — ACCEPTED AND DEFERRED → #2205

The observation is correct and I verified it against the code rather than taking it on faith: _dispatch reads photo[-1] per update and never references media_group_id, and transport.receive dispatches each update independently — so a four-screenshot album is four turns, with the caption on only one.

Worth noting for whoever picks it up: albums sometimes coalesce today, because a member arriving while a turn is running goes through the mid-turn queue and the drain collapses queued messages into one combined turn. That is a race, not the mechanism — it does not apply when the session is idle, so it cannot be relied on.

Deferring rather than fixing here, for the reason Design Review itself gave ("fine as a fast follow-up"): single-attachment ingestion is correct and complete without it, the fix lives entirely in Telegram envelope grouping and needs a debounce window plus a bounded buffer to be safe, and folding that into this PR would widen a green diff to satisfy an advisory note. Filed as #2205 with the implementation sketch and the two hazards (unbounded buffer, and how an over-cap album should degrade against IngestLimits.max_attachments).

No code change in this push; this is the written disposition only.

[chenmingwei23]

BLOCKING — transport_dispatch.py:626 — deferred attachments are never drained — FIXED in ede5f5da357489624da85ab1f453d32e7a4da797. Caused by my own previous fix, and the better catch of the two.

The cap I added last round deferred the overflowing album back onto the queue, but Telegram's _drain_queue was single-pass and the drained turn runs with drain=False — so the deferred album sat there until the user happened to send something unrelated. I traded a silent drop for a silent stall, which is arguably worse because the file looks accepted.

Ported Discord's shape properly: _drain_queue now wraps its body in while True, with if not texts: return as the terminator, so one burst pumps through as many cap-bounded turns as it needs and messages arriving mid-drain join the same FIFO pump. Also corrected the debug line, which still claimed the surplus waits "to the next turn".

The cap test pins both halves, and I reordered its assertions deliberately: cap-per-turn is checked before the turn count, because a cap regression merges both albums into one turn and would otherwise trip the pump-count assertion first, leaving the cap unpinned. Each mutation now fails on its own assertion:

• exceeds_attachment_cap = False -> turn 0 carried 20 attachments, over the cap of 10
• outer while True -> if True -> the drain must keep pumping: the deferred album has to run in a SECOND turn

One pre-existing test updated rather than deleted. test_drain_caps_collapse_and_defers_remainder asserted the surplus stays queued — the old single-pass contract this change deliberately removes. Rewritten as test_drain_caps_collapse_and_drains_remainder_in_order, keeping its real intent (surplus neither dropped nor reordered) and strengthening it: first turn takes exactly the 50-message cap in order, surplus [m50, m51] drains in a second turn in original order, queue ends empty.

client.py:331 shutdown finding — unchanged by this commit, authorized for override, tracked in #2217. Re-applying once this SHA is reviewed.

Gates on ede5f5da357489624da85ab1f453d32e7a4da797: isort, flake8, mypy (851 files), 624 tests. Rebased onto fresh main; three-dot diff verified at exactly this PR's 12 files.

[chenmingwei23]

Opus 5 advisory — telegram/client.py per-item album captions — FIXED in b552d6acbec81b77112e351f1e366a20cd411959. Correct catch; my comment even stated the wrong assumption out loud.

I had written "the caption rides on exactly one member" and coded next((m.text for m in members if m.text), "") to match. Telegram Desktop and Android both let a user caption individual items of a media group, so for a per-item album that silently dropped captions 2..N — the user's own words, never reaching the model, with the images still arriving so the turn looks complete. Same silent-drop family as the steer and cap findings, and the worst kind because nothing in the transcript shows the loss.

Now text = "\\n\\n".join(m.text for m in members if m.text), joined in album order. For the single-caption case this is byte-identical output, so the five existing caption tests stay valid unchanged rather than being rewritten to fit.

Pinned by test_every_per_item_caption_survives (caption on members 1 and 3, bare member in between). Mutation-verified — reverting the call site to next(...) fails on its own named assertion, every per-item caption must survive, in album order, with and this one visibly absent from the diff.

---

Design Review advisory — duplicated drain/collapse loop — ACCEPTED, tracked in #2304, not doing it here.

The observation is right and it lands on my own change: #2201 hoisted transcribe_audio_attachments into messaging/attachments.py to kill exactly this drift hazard, and then the cap/defer/pump loop reintroduced it one layer up.

It also has the strongest evidence behind it of anything raised on this PR. All three drain defects found during #2201/#2213 review were parity gaps — present in one channel, absent in the other:

defect	Discord	Telegram
attachment-bearing message takes steer path, drops files	had and not msg.attachments	missing
collapsed-attachment cap	had it	missing
single-pass drain stalls deferred album	already looped	missing

A shared implementation makes that class structurally impossible instead of something review has to re-catch per channel.

Deferring rather than doing it in this PR: it is a cross-channel refactor of the Discord path, which is live and carries its own test surface, on top of a change that has already grown to 12 files. Behaviour is correct in both channels today and both sides are mutation-verified. #2304 carries the proposed shape and the note that its contract test must be scoped to the function body — a module-scoped inspect.getsource assertion passes on the import line alone, which I already got wrong once on this PR.

Gates on b552d6acbec81b77112e351f1e366a20cd411959: isort (CI-exact, idempotent across two runs), flake8, mypy (851 files), 399 tests. Rebased onto fresh main 533323e37; merge-base equals main tip and the three-dot diff is exactly this PR's 12 files.

[chenmingwei23]

/ai-review override gpt b552d6a: Pre-existing shutdown loss class, not introduced here -- _closing already rejects any turn, so a plain single-photo message arriving in the same window is lost identically today; close() never awaits _handler_tasks, so the prescribed reorder cannot work without awaiting a full unbounded agent turn against the existing 2s teardown timeout, and durable inbound persistence (the real fix, needed for the single-message case too) is tracked in #2217.

[github-actions]

Human judgment recorded

@chenmingwei23 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for b552d6acbec81b77112e351f1e366a20cd411959.

Pre-existing shutdown loss class, not introduced here -- _closing already rejects any turn, so a plain single-photo message arriving in the same window is lost identically today; close() never awaits _handler_tasks, so the prescribed reorder cannot work without awaiting a full unbounded agent turn against the existing 2s teardown timeout, and durable inbound persistence (the real fix, needed for the single-message case too) is tracked in #2217.

This decision applies only to this commit. A new push requires a new judgment.

[chenmingwei23]

Design Review advisory on b552d6acbec81b77112e351f1e366a20cd411959 — sharpened duplication finding — ACCEPTED, scope verified and folded into #2304. Still a follow-up.

Opus is clean on this SHA (no findings — the per-item caption fix closed its only advisory), so this is the last open item and it is non-blocking.

I verified the claim rather than just agreeing with it, and it holds precisely: Mirrors discord/transport_dispatch.py appears exactly 5 times in telegram/transport_dispatch.py (85, 250, 482, 562, 581), spanning three distinct duplicated blocks — the drain/collapse/cap-deferral loop, the steer-path attachment gate, and the burst-collapse accounting.

One correction, in the direction of the finding being understated: it frames the risk as "the fourth channel," but there are already 7 dispatchers — discord slack teams telegram webex wecom weixin. Only Discord and Telegram set files_inbound=True, so only those two carry the gates (not msg.attachments exists solely at discord:218,535 and telegram:251,486). Every future file-capable channel starts from zero.

And this PR is the evidence for it. Telegram was written by hand-mirroring Discord and still shipped, round by round, missing four gates Discord already had — steer-path gate, command-path gate, collapsed-attachment cap, and pump-until-empty drain. Four for four, each surfaced by a different review round, each a silent drop or silent stall. Hand-mirroring demonstrably does not converge, which is a stronger argument for hoisting than the byte-similarity itself.

Not doing it here: it is a cross-channel refactor of the live Discord and Slack paths on top of a change already at 12 files, and behaviour is correct in both channels today with every gate mutation-verified. #2304 now carries the verified line references, the 7-dispatcher scope, the four-gate evidence table, and the note that its contract test must be function-body-scoped.

Final state of b552d6acbec81b77112e351f1e366a20cd411959: 48/48 checks, running=0 failing=0, readiness pass, 0 unresolved threads. GPT ✅ (override accepted, #2217) · Opus ✅ no findings · Design ✅ PASS · UX ✅. Every SHA-scoped reviewer marker matches this HEAD. Gates: isort (CI-exact, idempotent), flake8, mypy 851 files, 399 tests locally; full matrix green in CI including Backend Tests 3.10 ×4, 3.12 ×4, Windows ×4, E2E, Frontend, CodeQL, Build Wheel/Desktop.

mergeState remains BLOCKED on reviewDecision=REVIEW_REQUIRED — human approval under branch protection, the one gate I neither can nor should close. Auto-merge is not armed.