You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
add inbound and outbound file attachment support for the WeCom channel
download and decrypt inbound WeCom media using the SDK aeskey
upload and send outbound WeCom media with follow-up text handling
improve mixed message parsing and attachment-related test coverage
Details
This PR upgrades the WeCom channel from placeholder-only file handling to a more complete attachment flow covering both inbound and outbound scenarios.
Inbound changes
update flocks/channel/builtin/wecom/channel.py
preserve file metadata from inbound file and mixed messages
Good. Upgrades the WeCom [文件消息]/[图片消息] placeholders into actual local attachments and adds the missing send_media, mirroring the existing Feishu shape.
Compatibility
Doesn't break the Feishu path; deployments without the SDK degrade gracefully (warning only, not fatal).
Risk
Medium. file:// URL handling and the placeholder-rewrite rule in dispatcher.py are flawed; mixed-message coverage is incomplete; no obvious security issues.
Tests
Reasonably comprehensive — decrypt failure, size limit, mixed nested aeskey, .. filename traversal are all covered.
Recommendation
Request changes (fix the blocking items, then merge).
download_inbound_media produces the URL via Path.resolve().as_uri(), which percent-encodes the path. For example:
A Chinese filename or one containing spaces → file:///Users/x/.flocks/.../wx_1_%E6%8A%A5%E5%91%8A.pdf
After replace("file://", "") → /Users/x/.flocks/.../wx_1_%E6%8A%A5%E5%91%8A.pdf
Any agent that later open()s that path will fail.
Also the file:// scheme actually uses three slashes (file:///), so after replace we get a leading / that happens to look right on macOS/Linux but on Windows file:///C:/x becomes /C:/x, which is wrong.
None of these match. This is exactly the mixed scenario the PR specifically extends (the commit message even highlights mixed nested aeskey), yet the [文件: …] / [图片] placeholders will be passed verbatim to the agent and the Attached files:\n- /... rewrite never fires — inconsistent with the pure file/image case, and inconsistent with what the PR description claims.
Suggested fix — match a known set of placeholder substrings rather than equality:
Errors are silently dropped. If Message.parts or publish_event fails, there is zero trace in production. At minimum each except should log.warning(...).
publish_event is imported twice in adjacent blocks.
Hot-path readability._append_user_message is now ~90 lines. Extracting an _attach_inbound_media(...) helper would help.
Implicit cross-channel behavior. Previously this code was guarded by if msg.channel_id != "feishu". The new if not msg.media_url ... check means every channel runs the rewrite logic. Today it's harmless because Feishu doesn't produce these placeholder strings, but future channels could match by accident. Prefer an explicit if msg.channel_id == "wecom" guard around the rewrite, or have each channel return a list of placeholders to replace.
4. In send_media, a text failure marks the media send as failed
Both calls are inside the same try. If the companion text raises (e.g. rate limited), send_media returns success=False even though the media was already delivered. Worse, _send_with_retry upstream will then retry, and the same media will be sent again.
Side note: OutboundContext is a @dataclass, so {**vars(ctx), "media_url": None} works, but dataclasses.replace(ctx, media_url=None) is clearer and won't break if a frozen flag is added later.
Misbehaving proxies sometimes send non-numeric values; wrap the int() call in try/except (TypeError, ValueError).
9. _sanitize_filename is too aggressive on Chinese filenames
re.sub(r"[^A-Za-z0-9._-]+", "_", name.strip())
This collapses entire Chinese filenames into underscores, which is bad UX for enterprise users — the on-disk filename loses all signal about its origin. Feishu has the same long-standing issue, but since this PR significantly expands the amount of media being persisted to disk for WeCom, consider relaxing to \w or preserving Unicode (only stripping path separators /\, control chars, and ..).
Doesn't have to ship in this PR, but worth a follow-up issue.
10. Event payload should include messageID
The new FilePart reuses the message.part.updated event, which currently semantically overlaps "create" and "update". That's a pre-existing convention I won't push back on here. However, the published payload only carries sessionID and part. Adding messageID makes life much easier for the front-end:
_extract_sent_message_id only inspects body.msgid / body.message_id. Some WeCom SDK variants use body.msg_id; consider adding it for resilience.
prepare_wecom_media does not constrain file:// URLs, so theoretically a constructed file:///etc/passwd could be uploaded. In the current architecture media_url comes from agent output, so this is not external input — still, an allow-list/prefix check would harden it.
download_inbound_media writes to ~/.flocks/data/channel_media/wecom/<acct>/<YYYY-MM-DD>/ with no retention policy. Feishu has the same gap. A global cleanup job would be nice eventually.
send_media's vars(ctx) reconstruction → dataclasses.replace(ctx, media_url=None) is clearer and resists future field additions.
_close_api_client silently skips when the SDK doesn't expose _client.aclose; a one-time log.debug would help debugging.
Missing: in tests/channel/test_channel.py the dispatcher rewrite is asserted for the pure [文件消息: report.pdf] case, but there is no assertion for the mixed scenario (either "should be rewritten" or "should not match"). Please add one after fixing issue Fix: Change license #2.
Good test coverage; please add the two cases listed above.
Once the four blocking items are addressed, this is good to merge.
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
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.
Summary
Details
This PR upgrades the WeCom channel from placeholder-only file handling to a more complete attachment flow covering both inbound and outbound scenarios.
Inbound changes
update
flocks/channel/builtin/wecom/channel.pyupdate
flocks/channel/builtin/wecom/inbound_media.pyupdate
flocks/channel/inbound/dispatcher.pyFilePartand the rewritten text partOutbound changes
add
flocks/channel/builtin/wecom/media.pyupdate
flocks/channel/builtin/wecom/channel.pysend_media()support using WeCom upload + send/reply media APIsTests
expand
tests/channel/test_wecom.pyupdate
tests/channel/test_channel.pyWhy
Previously, WeCom file messages were reduced to placeholder text and outbound file sending was not supported.
This change makes WeCom attachments usable in both directions: