feat: receive images, voice and files on the Weixin channel

[chenmingwei23]

1. What is the problem?

Sending an image to the bot on WeChat did nothing. No reply, no error, no log
line — the message simply disappeared.

WeixinTransport.receive extracted only the ITEM_TEXT item from an inbound
iLink envelope and then dropped anything with no text:

# Text only for now — media (voice/file/image/video) is not supported yet.
text = ""
for item in raw_envelope.get("item_list") or []:
    if item.get("type") == ITEM_TEXT:
        text = (item.get("text_item") or {}).get("text", "")
        break
if not text:
    return

An uncaptioned screenshot has no ITEM_TEXT item, so it hit that return
before authorization, before dispatch, before any logging.

2. Why this issue matters to the user

The failure is silent on the side that matters. WeChat shows the image as sent,
the bot keeps its typing indicator, and the user waits for a reply to a message
the agent was never told about. Nothing distinguishes it from a slow turn.

It also removes the fastest way to give an agent context. This PR exists because
I hit it live: I needed to show the agent a screenshot of a WeChat chat, could
not, and had to leave WeChat and re-send it from the dashboard. On a phone that
is the difference between one gesture and abandoning the channel.

Voice notes and file attachments were dropped by the same branch.

3. How our fix solves it

Symptom — an image sent on WeChat is never answered.
Direct cause — if not text: return treats an empty text extraction as an
empty message.
Why the extraction is empty — iLink envelopes never carry media bytes. An
image arrives as a CDNMedia reference (encrypt_query_param + aes_key)
pointing at an AES-128-ECB-encrypted object on the WeChat CDN, in an
image_item, not a text_item.
Root cause — the transport had no CDN path at all, so the only item type it
could read was text, and emptiness was overloaded to mean "nothing arrived".

The fix is three separated layers:

• weixin/media.py — the protocol-shaped half, and nothing else: CDN
  download-URL construction with the query param percent-encoded (it is base64,
  so a raw + reads back as a space and a raw / as a path separator), key
  decoding, AES-128-ECB decrypt with PKCS7 unpadding, and a size cap enforced on
  bytes actually read rather than on Content-Length. ECB is dictated by the
  remote protocol, not chosen here; it is confined to this module and used only
  to read bytes WeChat already encrypted.

  aes_key carries two encodings for the same value — base64(raw 16 bytes) for images, base64(ascii hex) for file/voice/video — and iLink never
  says which. Discrimination is by decoded length plus a strict hex check,
  because guessing wrong produces plausible garbage rather than an error.

• weixin/attachments.py — maps the four CDN-backed item types onto the
  shared Attachment and delegates to messaging/attachments.py. Everything
  policy-shaped (classification, per-class size limits, image-signature
  validation, redaction, rejection wording, transcription, temp-file ownership)
  stays channel-neutral and behaves exactly as it does for Telegram and Discord.
  iLink labels no image format at all, so images are declared image/jpeg and
  the shared layer re-sniffs by magic bytes, rejects non-images (CWE-434), and
  renames the temp file to what it actually is.

  A voice item that already carries server-side text short-circuits the
  download entirely: iLink voice is SILK, which no shipped transcription backend
  decodes, so fetching it would spend a CDN round trip to produce
  "transcription failed" when the server already handed over a transcript.

• weixin/transport.py — collects media items into InboundMessage.attachments
  and only drops the envelope when there is neither text nor media.
  files_inbound flips to True; files_outbound stays False because the
  upload half (getuploadurl + encrypted CDN PUT) is unimplemented and the
  capability contract must not over-promise.

Ingestion is skipped while a turn is already live. Only the fresh-turn
session/prompt path inlines an image path as an image block; steer() sends
raw text and is fire-and-forget, so _handle_busy returns immediately and the
finally in transport_dispatch.py deletes the temp file before the in-flight
turn has read the steer — the model would receive a path to a file that no
longer exists. So a mid-turn sender is told to resend the attachment once the
reply finishes, and any caption beside it still reaches the running turn through
steer. An ingestion failure appends a visible [Attachment could not be read]
rather than reintroducing silence.

is_busy is checked twice, in _ingest_or_refuse: once before downloading
anything, and once after the download returns, because a CDN fetch takes real
time and a turn can start while it is in flight. On the second check the temp
files are discarded immediately rather than at the end of the frame, and only the
original caption carries on. There is no suspension point between that check and
_drive's own one, so the two cannot disagree — the window is closed, not
narrowed. On the path that does ingest, temp files are cleaned up in a finally.

The decrypted payload is written through asyncio.to_thread: a CDN object can
be 32 MB and TMPDIR is not guaranteed to be local disk, so writing it inline
would stall the single gateway event loop — and the liveness heartbeat with it —
for the duration of the write. Mirrors the Telegram and Discord callbacks.

cryptography is declared in install_requires rather than an extra, for the
same reason qrcode[pil] already is: the Weixin channel ships enabled-by-config
and must not depend on another feature's dependency tree to read a message. The
two ECB call sites carry an inline CodeQL suppression next to the existing
bandit one — ECB is what the WeChat CDN already encrypted with, and we never
encrypt our own data with it, so there is no algorithm choice to make.

4. What tests we did

37 new tests in test/test_weixin_media.py and 5 in
test/test_weixin_dispatch.py, all green, plus 208 green across the Weixin,
shared-messaging and attachment suites (the 2 local reds are
test_weixin_qr.py, which needs qrcode that this host lacks — green in CI).

Eight mutations verified — each breaks production code and the named test fails:

Mutation	Test that catches it
restore if not text: return	test_media_only_message_is_dispatched_not_dropped
32-byte key truncated to 16 instead of hex-parsed	test_thirty_two_non_hex_bytes_are_rejected_not_truncated
PKCS7 unpadding removed	test_exact_block_multiple_plaintext_survives_pkcs7
CDN param not percent-encoded	test_base64_query_param_is_percent_encoded
write inline instead of asyncio.to_thread	test_the_decrypted_write_never_runs_on_the_event_loop_thread
voice short-circuit back to per-message	test_a_transcribed_voice_does_not_suppress_an_untranscribed_sibling
drop the mid-turn refusal	test_a_mid_turn_attachment_is_refused_instead_of_ingested
drop the post-download busy recheck	test_a_turn_starting_during_the_download_still_refuses_the_attachment

The offload test asserts by thread identity, not by naming
asyncio.to_thread: an inline open()/write() runs on the loop thread, which is
the actual failure, and this catches it however it is spelled.

Gates: pytest, isort, flake8 clean. mypy clean on src/kiro_crew/weixin/ — the
two remaining errors are in src/kiro_crew/transcribe.py and reproduce
identically on main at the same count. No frontend changes, so tsc/vitest are
untouched.

test_declared_capabilities_do_not_promise_files_without_a_media_path was
rewritten rather than deleted: its invariant (a files flag must track a real
code path) still holds and is now asserted per direction.

Not yet verified end to end. The real CDN round trip is unexercised, because
the live gateway holds the only iLink long-poll for this bot account and a
second client would steal it. First real image after cutover is the remaining
check.

5. Any other suggestions on the work

Two findings from the investigation that led here, neither addressed by this PR:

Streaming is not available on iLink, and neither is tool-call progress. The
protocol exposes five endpoints (getupdates, sendmessage, getuploadurl,
getconfig, sendtyping) with no edit/update primitive, and upstream
@tencent-weixin/openclaw-weixin hardcodes MessageState.FINISH on all three
send paths, so GENERATING is inbound-only. MessageItemType also carries
undocumented TOOL_CALL_START: 11 / TOOL_CALL_RESULT: 12, which upstream 2.4.4
sends by default — I probed both against a live bot: HTTP 200, empty body, no
message_id, and nothing rendered in the client. Declaring channel_version 2.4.6 changed nothing. Worth an issue recording the dead end so the next person
does not repeat it; the typing indicator remains the only progress affordance.

Outbound media is the natural follow-up. Everything needed is already here
(CDN helpers, AES key handling); it needs getuploadurl, the encrypted PUT, and
files_outbound=True. Sending screenshots to the user would close the loop.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've inspected the full diff and the surrounding contracts (shared attachment pipeline, command parsing, the double busy-check flow, CDN download/decrypt, authorization ordering). Let me verify one concern: the AES decrypt runs synchronously in download_media before the offloaded write, and whether authorization gates any CDN fetch.

The download only happens post-authorization (via dispatch), the CDN host is fixed (no SSRF), the double is_busy check has no intervening suspension point (single-loop, correct), temp-file cleanup is in finally, and the media-only drop condition (not text and not media_items) correctly closes the original regression. The decrypt is CPU-bound (not a blocking syscall) and the real IO (write) is offloaded per the loop rule.

No findings.

[OPUS-REVIEWED] 8ab731a

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8ab731a873835818bd5453e48df1b8b47c71f89a and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8ab731a

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root cause fixed at the right layers — protocol in media.py, adapter in attachments.py, policy stays channel-neutral — mirroring the Telegram/Discord shape exactly.

Suggestions

• Mid-turn attachments are refused with "resend later" while Telegram queues them through the drain; once Weixin grows a queue, carry attachments through it — worth a tracked follow-up so the channel-UX divergence doesn't ossify.
• transport.py's scaffold note says video is "downloaded-then-rejected", but the shared pipeline classifies and rejects VIDEO before invoking the download callback — correct the note so nobody "optimizes" a download that never happens.

[DESIGN-REVIEWED] 8ab731a

[chenmingwei23]

Round 1 dispositions — pushed as 30b9e619c. All four findings accepted as real; none rebutted.

GPT + Opus (blocking) — synchronous write of up to 32 MB on the event loop. Fixed. _download now writes via await asyncio.to_thread(Path(dest).write_bytes, data), matching the Telegram/Discord callbacks and the _make_temp offload already in _fetch. Guarded by test_the_decrypted_write_never_runs_on_the_event_loop_thread, which asserts on thread identity rather than on the call being spelled asyncio.to_thread — an inline open()/write() puts the write on the loop thread, which is the actual failure. Mutation-verified: restoring the inline write fails that test.

GPT (blocking) + Design Review (Watch) — mid-turn attachments become dangling paths. Fixed, and both of you were right about the mechanism I got wrong: steer() sends raw text, never build_prompt_blocks, and it is fire-and-forget, so my finally deleted the temp file before the in-flight turn read the steer. handle_message now checks is_busy before ingesting: when a turn is live the attachments are not downloaded at all, the sender is told to resend after the reply, and any caption beside the image still reaches the running turn through steer (so a mid-turn caption is not swallowed either). Three tests cover it — refusal, caption-still-steered, and media-only-ends-after-refusal — plus one that the idle path still ingests. Mutation-verified. The PR body's "runs before the busy check" paragraph was wrong and has been rewritten.

Design Review (Suggestion) — all-or-nothing voice short-circuit. Fixed. _voice_transcripts(items) -> list[str] became _voice_transcript(item) -> str | None, and the skip is now keyed on the item's index, so a transcript on one voice note no longer suppresses the download of a sibling that came without one. test_a_transcribed_voice_does_not_suppress_an_untranscribed_sibling sends two voice items where only the first carries server text and asserts only the second was fetched. Mutation-verified.

Two CI reds fixed alongside:

• test_pip_deps_consistency — cryptography was an unguarded module-level import in weixin/media.py and undeclared, so a pip install would have crashed on the first inbound image. Declared in install_requires rather than an extra, for the same reason qrcode[pil] already is: the Weixin channel ships enabled-by-config and must not depend on another feature's dependency tree to read a message.
• CodeQL py/weak-cryptographic-algorithm (2 high) — the ECB call sites in media.py and its test inverse. Suppressed inline with # lgtm[py/weak-cryptographic-algorithm] beside the existing # nosec B305, with a comment stating why there is no choice to make: the CDN hands us objects it already encrypted this way and we never encrypt data of our own with it. Same suppression form the repo already uses for py/path-injection and py/clear-text-storage-sensitive-data.

The Coverage Gate red was downstream of the backend-test failure (backend-test=failure -- failing closed) and needs no separate change.

Still unverified end to end: the real CDN round trip. The live gateway holds the only iLink long-poll for this bot account, so the first real image after cutover remains the outstanding check.

[chenmingwei23]

Round 2 dispositions — pushed as d1cf728f2.

GPT (blocking) — session can become busy during ingestion. Fixed, and a real hole I left open: the pre-ingest check only covers a turn that was already live. A CDN download takes real time, so a turn can start while it is in flight, and the already-downloaded path would then be inlined into a steer whose file this frame deletes — the exact failure the first check exists to prevent.

handle_message's attachment block moved into _ingest_or_refuse, which checks is_busy twice: once before downloading anything, and once after the download returns. On the second check the temp files are discarded immediately rather than at the end of the frame, and only the original caption is carried on — append_attachment_context is never applied. There is no suspension point between that second check and _drive's own is_busy check, so the two cannot disagree, which closes the window rather than narrowing it.

test_a_turn_starting_during_the_download_still_refuses_the_attachment flips is_busy to True from inside the ingestion stub and asserts three things: the steer carries only "what is this error", the image path appears nowhere in it, and the downloaded file no longer exists. Mutation-verified — deleting the recheck fails that test.

CodeQL py/weak-cryptographic-algorithm (2 high). Dismissed as won't fix, alerts #595 (weixin/media.py) and #596 (its test inverse), each with the reason recorded on the alert.

The inline # lgtm[...] comment I added in round 1 does not suppress anything — CodeQL default setup does not honour lgtm comments, which is why the alerts came back at the new line numbers. Dismissal is this repo's actual mechanism for exactly this shape: there are already dismissed py/path-injection alerts on files.py / file_explorer/server.py and py/clear-text-storage-sensitive-data on history.py / archive.py, each sitting next to the same style of documenting comment. The comments stay for the human reader; the alert state is what the gate reads.

Substance of the dismissal: ECB is dictated by the remote protocol. Inbound iLink media arrives already AES-128-ECB-encrypted under a per-file random key and the CDN accepts nothing else, so there is no algorithm choice to make. It is confined to weixin/media.py, used only to read bytes WeChat encrypted, and never to protect data of our own. The test alert is the fixture's inverse of that same decrypt — encrypting the fixture any other way would not exercise the path under test.

Coverage Gate / PR Readiness were downstream of the reds above and need no separate change.

[chenmingwei23]

Round 3 disposition — pushed as bb866cfac.

GPT (blocking) — command-like captions silently discard attachments. Real, and the same silent-drop class this PR exists to remove, so it is fixed. I did not take the literal suggested fix ("move attachment ingestion before command parsing"), because on that path it makes things worse rather than better: parse_command matches only when the entire stripped text is an alias, so /new with an image is unambiguous command intent, and the command path runs no turn at all. Ingesting first would spend a CDN round trip, write temp files, and then discard them with the model still never seeing the image — a wasted fetch and a new cleanup obligation, for the same outcome.

What the finding actually identifies is the silence, not the skipped download. So the command intercept now names it: when a command message carries media, the user is told 📎 附件未读取：这条是命令消息，请把附件单独发送。 before the command's own ack, and the command behaves exactly as before. No download, no temp files, no silent loss.

Two tests: test_an_attachment_riding_a_command_is_named_not_dropped asserts the reset still happens, the notice is sent, and — via a stub that raises if called — that no CDN round trip is spent; test_a_plain_command_says_nothing_about_attachments keeps the notice scoped to messages that actually carried media. Mutation-verified — deleting the notice fails the first test.

If you would rather an image captioned /new be treated as a normal message (image ingested, /new passed through as literal text) instead of as a command, say so and I will flip it — but that trades away the ability to reset while sending an image, and parse_command's exact-match shape reads as deliberate command intent to me.

CodeQL is green at this SHA — the two ECB alerts (#595, #596) are dismissed with reasons recorded, per round 2. 120 local tests green; isort/flake8 clean; mypy clean on the changed file (its 2 errors are pre-existing in transcribe.py and reproduce on main).

[chenmingwei23]

Round 4 — no code change; the last remaining red is not this PR's.

Backend Tests (Windows) (2) — inherited from main, not caused by this branch. The failure is test/test_dashboard_chat_pins.py::test_load_transient_io_error_preserves_existing_state → Failed: DID NOT RAISE <class 'OSError'>.

Evidence it is inherited rather than mine:

• It fails on main itself: run 31347030698, job Backend Tests (Windows) (2) — the only failing job in that run, same test, same message.
• This PR's diff is entirely under src/kiro_crew/weixin/ plus setup.cfg and docs/system-specs/modules/messaging.md. test/test_dashboard_chat_pins.py does not exist on this branch at all — it reaches CI through the merge ref.
• Linux and macOS shards pass here; only the Windows shard is red, and only on that one test.

Filed as #2461 with the root-cause leads (the test injects the I/O error by patching read_text on the concrete pathlib class, which apparently does not intercept what load_chat_pins() actually calls on Windows — the same class of platform difference the test's own comment says it was written to avoid). Not fixed here: it is unrelated to this change, and folding a dashboard-state test fix into a Weixin media PR would be the wrong scope.

I re-ran the failed shard on this PR in case it is intermittent rather than deterministic; if it comes back red, #2461 is the owner.

Everything else at bb866cfac is green — 47 checks pass, all four review bots PASS, CodeQL green after the ECB dismissals (#595/#596), MERGEABLE. No open findings from any bot or human.

[chenmingwei23]

Ready for approve at bb866cfac.

47 checks pass. All four review bots PASS with zero open findings: GPT 5.6 ✅, Opus 4.8 ✅, Design Review (Fable 5) ✅, UX Review ✅. CodeQL green. MERGEABLE.

The one remaining red is Backend Tests (Windows) (2), and it is not this PR's. test_dashboard_chat_pins.py::test_load_transient_io_error_preserves_existing_state fails identically on main (run 31347030698, the only failing job there), and that file does not exist on this branch — it reaches CI through the merge ref. I re-ran the shard once; it failed the same way, so it is deterministic on Windows rather than flaky and a re-run cannot clear it. Tracked as #2461 with the root-cause lead. PR Readiness's single blocking item is that shard, so it clears when #2461 does.

Three review rounds, all findings accepted as real, none rebutted:

Round	Finding	Fix
1	GPT + Opus: up-to-32 MB synchronous write on the gateway event loop	asyncio.to_thread(Path(dest).write_bytes, ...)
1	GPT + Design Review: mid-turn attachment becomes a path to a deleted file	no ingestion while a turn is live; sender told to resend, caption still steered
1	Design Review: all-or-nothing voice short-circuit	per-item, keyed on the item's index
2	GPT: session can become busy during the CDN download	second is_busy check after the download, temp files discarded immediately
3	GPT: an image captioned /new silently discarded	command intercept names the skipped attachment; no CDN round trip spent

Plus two CI reds fixed: cryptography was an undeclared unguarded import (a fresh pip install would have crashed on the first inbound image), and the two protocol-dictated ECB alerts are dismissed with reasons recorded (#595, #596).

Eight mutations verified — each one breaks production code and fails its named test.

Still unverified end to end: the real CDN round trip. The live gateway holds the only iLink long-poll for this bot account, so a second client would steal it. The first real image after cutover is the outstanding check.

[chenmingwei23]

Correction to my round-4 comment, and a rebase: 8ab731a87.

I called the Backend Tests (Windows) (2) red "inherited and deterministic" and filed #2461 for it. That was wrong in a way worth stating plainly: the red was stale, not unfixable.

• The main run I cited as proof (31347030698) ran at bdd55fc76, where the test still injected the I/O error with chmod(0o000) — the version that cannot fail on Windows, since Windows ignores POSIX permission bits for read access.
• test(dashboard): make chat-pins transient-I/O test portable on Windows #2454 replaced that with the read_text patch and merged at 02:08:24Z.
• This PR's CI run was created at 01:42:20Z, so its merge ref predates test(dashboard): make chat-pins transient-I/O test portable on Windows #2454. The shard re-run I triggered re-ran inside that same stale run, which is why it failed identically — I read that repetition as determinism when it only showed the run was pinned to pre-fix code.
• I had read the test at ref=main after the merge, so the read_text code I quoted and criticised was in fact test(dashboard): make chat-pins transient-I/O test portable on Windows #2454's fix, not the bug.

Rebased onto current main (e7f13c2d3, which contains #2454); the branch is now 0 commits behind. #2461 closed as already fixed.

Re-verified locally after the rebase: 204 tests pass across the Weixin, chat-pins, and pip-deps suites; isort and flake8 clean. The commit content is unchanged from bb866cfac — this is purely a base move.

[chenmingwei23]

Fully green at 8ab731a87 — ready for approve.

49/49 checks pass, zero reds, PR Readiness green, MERGEABLE. All four review bots PASS with no open findings: GPT 5.6 ✅, Opus 4.8 ✅, Design Review (Fable 5) ✅, UX Review ✅.

The Backend Tests (Windows) shards all pass now, which confirms that red was stale rather than inherited: it was the pre-#2454 chmod(0o000) version of test_load_transient_io_error_preserves_existing_state riding along in a merge ref computed before #2454 merged. The rebase picked up the fix. #2461 is closed as invalid.

Summary of what landed, for the reviewer:

Round	Finding	Resolution
1	GPT + Opus: up-to-32 MB synchronous write on the gateway event loop	asyncio.to_thread(Path(dest).write_bytes, ...)
1	GPT + Design Review: mid-turn attachment handed the model a deleted path	no ingestion while a turn is live; sender told to resend, caption still steered
1	Design Review: all-or-nothing voice short-circuit	per item, keyed on the item's index
2	GPT: session could become busy during the CDN download	second is_busy check after the download; temp files discarded immediately
3	GPT: image captioned /new silently discarded	command intercept names the skipped attachment; no CDN round trip spent

Plus cryptography declared in install_requires (an unguarded module-level import would have crashed a fresh pip install on the first inbound image), and the two protocol-dictated ECB CodeQL alerts dismissed with reasons recorded (#595, #596).

Eight mutations verified — each breaks production code and fails its named test.

Outstanding, and not something CI can cover: the real CDN round trip is unexercised. The live gateway holds the only iLink long-poll for this bot account, so a second client would steal it. The first real image after cutover is the remaining check.