Skip to content

feat(gateway): add WeCom (企业微信) channel adapter#769

Open
canyugs wants to merge 22 commits intoopenabdev:mainfrom
canyugs:feat/wecom-adapter
Open

feat(gateway): add WeCom (企业微信) channel adapter#769
canyugs wants to merge 22 commits intoopenabdev:mainfrom
canyugs:feat/wecom-adapter

Conversation

@canyugs
Copy link
Copy Markdown
Contributor

@canyugs canyugs commented May 7, 2026

Summary

Adds WeCom (企业微信) support to the Custom Gateway. Users can chat with OAB agents via WeCom enterprise app 1:1 direct messages. Group chat is not supported by the WeCom self-built app callback API and is documented as a non-goal of this PR — see Not Yet Supported.

WeCom ──POST──▶ ┌──────────────────┐
  (callback)    │  Custom Gateway  │ ◀──WebSocket── OAB Pod
                │     :8080        │   (OAB connects out)
WeCom ◀─reply── └──────────────────┘
  (send API)

Prior Art & Industry Research

How comparable open-source agent gateways handle WeCom integration:

OpenClaw (repo) — delivers WeCom support as an out-of-tree community npm plugin: @wecom/wecom-openclaw-plugin, maintained by the Tencent WeCom team. The plugin uses WeCom Bot WebSocket persistent connections (not HTTP callback mode), supporting DMs and group chats. OpenClaw's core never ships a callback-mode adapter — it relies on the plugin catalog model.

Hermes Agent (repo) — ships two in-tree WeCom adapters:

  1. gateway/platforms/wecom.py — bot/WebSocket mode via WeCom AI Bot gateway (introduced v0.6.0 / PR #3847).
  2. gateway/platforms/wecom_callback.py — self-built enterprise app callback mode (introduced v0.9.0 / PR #7943). This is the closest comparator to this PR.

Hermes' callback adapter has a few notable choices that informed (or contrast with) this PR:

Concern Hermes wecom_callback.py This PR
Crypto WXBizMsgCrypt helper class wrapping AES-CBC + SHA1 sig Inline AES-256-CBC + SHA1 sig in wecom.rs
Multi-app Yes — apps: List[...], scoped by corp_id:user_id No — single app only (deferred)
Dedup TTL 300s (MESSAGE_DEDUP_TTL_SECONDS) 30s (WeCom retries within ~5s)
Token TTL 7200s 7100s (100s safety margin)
Default port/path 8645 / /wecom/callback 8080 / /webhook/wecom (matches existing gateway adapters)
Group support callback adapter hardcodes chat_type="dm" (group routing only via separate wecom.py bot adapter) Not supported (matches Hermes' callback adapter)
Streaming "streaming cursor" + atomic state persistence Thinking placeholder + 3s debounce + recall

Neither project covers OpenAB's specific need — a single-binary Rust adapter inside the Custom Gateway that connects via WebSocket out to OAB pods. Hermes is Python/aiohttp; OpenClaw delegates to a separate Node.js plugin process.

Why This Approach

  • In-tree adapter (not plugin) — OpenAB's gateway is a single Rust binary that proxies events to OAB pods via WebSocket. A separate plugin process (OpenClaw model) would add deployment complexity (extra container, lifecycle management) for the same end result. In-tree matches existing adapters (feishu.rs, telegram.rs, googlechat.rs).
  • Self-built app callback (not WeCom AI Bot WS) — WeCom offers two integration paths: self-built enterprise apps (HTTP callback, used here) and AI Bot (WebSocket, used by OpenClaw and Hermes). Self-built apps expose the full WeCom enterprise feature surface (OAuth, contacts, calendar, approval workflows) that an OAB agent might want to call; AI Bot is purpose-built for chatbot use only. Both are generally available — no whitelisting. A WeCom AI Bot WS adapter is a sensible follow-up that would natively support group chats and streaming, but is out of scope for this PR.
  • AES-256-CBC inline (no helper crate) — WeCom's encryption format is well-specified (CBC + PKCS#7 + Corp ID suffix), and pulling in a full WXBizMsgCrypt-style helper would require taking on a new crate dependency for ~50 lines of crypto. Using aes/cbc from RustCrypto keeps the dep surface minimal and auditable.
  • Thinking-placeholder + recall (not edit-message) — WeCom callback mode has no "edit message" API (Feishu does, which is why feishu.rs uses incremental edits). The placeholder→debounce→recall pattern delivers streaming-feeling output without spamming users with N partial messages. Note: this causes a brief flicker on the WeCom client (recall + new message) — see chaodu-agent's UX feedback; tracked as a follow-up to make streaming opt-in.
  • 30s dedup window (not 300s) — WeCom retries failed callbacks within ~5 seconds and stops. A 30s window with double-checked locking covers the retry burst without unbounded memory growth. Hermes' 300s is conservative for their multi-app scenario; for single-app we don't need it.

Alternatives Considered

  1. Plugin out-of-tree (OpenClaw model) — rejected: adds a Node.js or sidecar process to the gateway pod, complicates Helm chart, and prevents sharing the gateway's existing WS-out-to-OAB transport. Worth revisiting if/when OpenAB grows a plugin runtime.
  2. WeCom AI Bot WebSocket mode (Hermes' wecom.py, OpenClaw plugin) — deferred to follow-up: AI Bot is generally available (no whitelist) but is a separate WeCom product type with a narrower feature surface than self-built apps. The protocol (aibot_subscribe / aibot_msg_callback / aibot_send_msg / etc.) requires its own ~600-line implementation including reconnect/heartbeat/anti-kick logic. Worth doing as a second adapter — it natively supports group chats and streaming — but doubles the surface area of one PR. Tracked as separate work.
  3. Use a third-party WeCom Rust crate — surveyed wecom-rs and similar; all are unmaintained (>2 years no commits) or only cover the messaging-send side, not callback decryption. Not safe to depend on.
  4. Multi-app routing in this PR — deferred: the codebase doesn't yet have a precedent for multi-tenant adapter config (other adapters are single-tenant), and adding it would expand scope. Tracked as follow-up.
  5. Markdown card replies — deferred to a follow-up PR (listed in "Not Yet Supported"); requires a separate render pipeline and isn't blocking the text path.

Changes

File Change Description
gateway/src/adapters/wecom.rs NEW Full WeCom adapter: AES-256-CBC message decryption, XML parsing, callback URL verification (echostr), access token caching with auto-refresh, message deduplication (30s TTL, 10K entry cap), image receiving (download + compress/resize), text file receiving via media API, streaming via thinking placeholder + 3s debounce flush + recall, long message splitting at UTF-8 char boundaries (2048-byte limit), 21 unit tests
gateway/src/adapters/mod.rs MOD Add pub mod wecom
gateway/src/main.rs MOD AppState + reply router + WecomAdapter initialization, env var detection for startup warnings
gateway/README.md MOD Add WeCom env vars and /webhook/wecom endpoint docs
gateway/Cargo.toml MOD Add aes, cbc, base64 crypto dependencies
docs/wecom.md NEW Operator guide: prerequisites, app creation, callback setup, env vars, K8s/Docker deployment, HTTPS exposure, troubleshooting
charts/openab/values.yaml MOD Add wecom config block under gateway
charts/openab/templates/gateway.yaml MOD Inject WeCom env vars (secrets via secretKeyRef, CORP_ID and AGENT_ID as plain values)
charts/openab/templates/gateway-secret.yaml MOD Add wecom-secret, wecom-token, wecom-encoding-aes-key to unified Secret
.github/workflows/ci.yml MOD Trigger CI on gateway/** and add a gateway job running cargo check + cargo test for the gateway crate (was previously skipped)
gateway/src/schema.rs MOD Add Default derive to Content and Attachment so future field additions don't require updating every adapter test fixture

Key Design Decisions

  1. AES-256-CBC decryption — WeCom encrypts all callback payloads with a 43-char EncodingAESKey (base64-decoded to 32 bytes). The adapter decrypts in-process using aes/cbc crates, validates Corp ID in the decrypted suffix, and strips PKCS#7 padding. No external crypto dependencies required.

  2. Streaming via thinking placeholder + debounce flush — WeCom has no edit-message API (unlike Feishu). Streaming uses a "thinking…" placeholder sent immediately, then a background task with a debounce timer (3s quiet period, 5min max wait). On flush: recall the placeholder, split the accumulated text, send final chunks. Uses tokio::sync::watch for efficient signaling.

  3. Message splitting at UTF-8 char boundaries — WeCom silently truncates messages exceeding 2048 bytes. split_text_lines() splits at newline boundaries, and for single lines exceeding the limit, splits at safe UTF-8 char boundaries to prevent mid-character truncation.

  4. Access token caching — Tokens are cached with 7100s TTL (WeCom issues 7200s tokens). Auto-refresh on expiry. Thread-safe via tokio::sync::RwLock.

  5. Image receiving with resize — Downloads media via WeCom's /cgi-bin/media/get API, detects content type, compresses/resizes images > 1MB to fit within OAB's processing limits. Sends as image attachment type in the gateway event schema.

  6. Text file receiving — Downloads files via media API, detects text files by extension (aligned with OAB's src/media.rs allowlist: .txt, .md, .rs, .py, .js, .ts, .json, .yaml, .toml, .css, .html, .sh, etc.) and special filenames (Dockerfile, Makefile, etc.). Sends content as text_file attachment type.

  7. DM-only scope — WeCom self-built enterprise app callbacks deliver only 1:1 member-to-app messages, not group chat messages. The adapter does not attempt group routing or @mention gating; group chat support belongs in a future WeCom AI Bot WS adapter (which natively supports it).

Testing

Scenario Result
Callback URL verification (echostr decrypt) PASS
AES-256-CBC message decrypt + XML parse PASS
Text message send/receive (DM) PASS
Image message receive → agent reply PASS
Text file receive (.rs, .md, .json) ⚠️ WeCom platform limitation — enterprise apps don't receive file callbacks
Message deduplication (duplicate msg_id) PASS
Long message splitting (>2048 bytes) PASS
UTF-8 char boundary splitting PASS
Streaming: placeholder → debounce → flush PASS
Multi-chunk streaming flush PASS
Access token refresh PASS
cargo test — 21 wecom tests passed (now wired into PR-time CI) PASS

Note: Text file receiving is implemented and tested via unit tests, but WeCom's enterprise app callback does not forward file type messages in practice (platform limitation). Image and text messages work as expected.

Configuration

# Helm values.yaml
agents:
  kiro:
    gateway:
      enabled: true
      deploy: true
      url: "ws://openab-kiro-gateway:8080/ws"
      wecom:
        corpId: "ww1234567890abcdef"
        agentId: "1000002"
        secret: "your-app-secret"
        token: "your-callback-token"
        encodingAesKey: "your-43-char-encoding-aes-key"

Environment Variables

Variable Required Description
WECOM_CORP_ID Yes Enterprise Corp ID
WECOM_AGENT_ID Yes App Agent ID
WECOM_SECRET Yes App Secret
WECOM_TOKEN Yes Callback verification Token
WECOM_ENCODING_AES_KEY Yes Callback EncodingAESKey (43 chars)
WECOM_WEBHOOK_PATH No Webhook path (default: /webhook/wecom)

Not Yet Supported

  • Group chat — WeCom self-built app callback delivers only DM messages. Group chat would require either the appchat API (group routing on send) plus group event subscription, or a separate WeCom AI Bot WebSocket adapter (which has native group support). Tracked as follow-up.
  • Voice/video messages — WeCom supports voice (AMR) and video callbacks; OAB has STT infrastructure but no gateway-side audio pipeline yet
  • Markdown card replies — WeCom supports interactive card messages; could provide richer formatting than plain text
  • File sending — Sending files/images back to users (currently text-only replies)
  • Customer service mode — Alternative API for external customer-facing bots (different auth flow)
  • Streaming opt-in — current default is on; recall+resend causes a brief client flicker. Per chaodu-agent's review, making this opt-in (default off) is tracked as follow-up.

Test plan

  • CI build + cargo test passes (21 wecom-specific tests, now wired into PR-time CI)
  • Deploy to Zeabur and verify DM text messages work end-to-end
  • Verify callback URL verification handshake with WeCom console
  • Verify image receiving triggers agent with image attachment
  • Verify streaming placeholder + debounce flush delivers complete responses
  • Verify long messages (>2048 bytes) split correctly without truncation

🤖 Generated with Claude Code

Discord Discussion URL

https://discord.com/channels/1491295327620169908/1501052008868745347

Closes #724

canyugs and others added 8 commits May 7, 2026 15:06
Implement WeCom as a new gateway platform for receiving and sending
messages via enterprise app callback API.

Features:
- AES-256-CBC message decryption (WeCom uses PKCS7 block_size=32)
- SHA1 signature verification with constant-time comparison
- Access token cache with auto-refresh and expiry margin
- Message deduplication (30s TTL, 10k max entries)
- Long message splitting at line boundaries (2048 byte limit)
- GatewayResponse with message_id for OAB core integration

Env vars: WECOM_CORP_ID, WECOM_SECRET, WECOM_TOKEN,
WECOM_ENCODING_AES_KEY, WECOM_AGENT_ID, WECOM_WEBHOOK_PATH

Note: Streaming (edit_message) is intentionally disabled — WeCom has
no native message edit API; recall+resend shows disruptive notifications.
Uses plain text msgtype since WeCom app message markdown is too limited
(no code blocks, tables, or lists).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add comprehensive WeCom setup documentation covering:
- Prerequisites and enterprise app creation
- Callback URL configuration
- Environment variables reference
- Docker/Kubernetes deployment
- Troubleshooting guide

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add handle_edit_message and recall_message methods to support OAB streaming.
When OAB sends edit_message commands, the adapter recalls the previous
message and re-sends updated content, tracking message ID changes via
msg_id_map to handle the WeCom limitation of no native edit API.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Download images from WeCom PicUrl, resize/compress via image crate,
and forward as base64 attachment in gateway event. Also fix upstream
googlechat test compilation (missing attachments field).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…r + debounce flush

Instead of recalling and resending on every streaming update, send a
single "⏳..." placeholder and buffer all edits. After 3 seconds of
inactivity, recall the placeholder once and send the complete response.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Download files via WeCom media API and forward text-based files
(code, config, data files) as text_file attachments to OAB.
Also fix split_text tests to match renamed split_text_lines function.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add 5-minute max timeout to debounce flush task to prevent leaks
- Make WECOM_AGENT_ID required (was defaulting to "0")
- Align Helm chart: require token + encodingAesKey in $hasWecom condition
- Fix README: mark WeCom env vars as required
- Update docs/wecom.md feature matrix to reflect implemented features
- Add WeCom vars to "no adapters configured" warning message

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ogging

split_text_lines now handles single lines exceeding the limit by
splitting at UTF-8 char boundaries. Previously long lines were sent
as-is, causing WeCom to silently truncate messages.

Also add detailed logging to flush_thinking for easier debugging.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 7, 2026 16:10
@canyugs canyugs requested a review from thepagent as a code owner May 7, 2026 16:10
@github-actions github-actions Bot added closing-soon PR missing Discord Discussion URL — will auto-close in 3 days pending-screening PR awaiting automated screening labels May 7, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new WeCom (企业微信) channel adapter to the openab-gateway service, including callback verification/decryption, token caching, message deduplication, and inbound media handling, plus documentation and Helm wiring so the adapter can be enabled via deployment config.

Changes:

  • Implement WeCom adapter (webhook verify + message callback, AES-CBC decrypt, token cache, dedupe, reply sending + streaming placeholder flush).
  • Wire the adapter into the gateway router/state and update gateway/environment documentation.
  • Add Helm values/templates and a new setup guide (docs/wecom.md) for configuring WeCom.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
README.md Adds WeCom entry pointing to the setup guide and gateway requirement.
gateway/src/main.rs Registers WeCom adapter, routes, and reply dispatch plumbing.
gateway/src/adapters/wecom.rs New WeCom adapter implementation (crypto, webhook handling, media download, streaming).
gateway/src/adapters/mod.rs Exposes the new wecom adapter module.
gateway/src/adapters/googlechat.rs Updates tests to include attachments field in Content.
gateway/README.md Documents WeCom env vars and webhook endpoints.
gateway/Cargo.toml Adds sha1 + quick-xml dependencies for WeCom integration.
gateway/Cargo.lock Updates lockfile for new deps.
docs/wecom.md New WeCom setup and usage documentation.
charts/openab/values.yaml Adds WeCom gateway values stanza.
charts/openab/templates/gateway.yaml Injects WeCom env vars into gateway deployment.
charts/openab/templates/gateway-secret.yaml Stores WeCom secrets in the gateway secret when configured.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

let pad_byte = *plaintext.last().ok_or_else(|| anyhow::anyhow!("empty plaintext"))? as usize;
if pad_byte == 0 || pad_byte > 32 || pad_byte > plaintext.len() {
anyhow::bail!("invalid wecom padding value: {pad_byte}");
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5 — now validates that all padding bytes equal the pad_byte value before stripping.

Comment on lines +166 to +168
if entries.len() >= DEDUPE_MAX_SIZE {
entries.retain(|_, t| now.duration_since(*t).as_secs() < DEDUPE_TTL_SECS);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix — the dedupe map has a 30s TTL and only triggers cleanup at 10,000 entries. WeCom enterprise app callbacks are rate-limited by the platform. Under realistic traffic this won't grow unbounded. Adding an LRU dependency for this edge case is over-engineering.

Comment thread gateway/src/adapters/wecom.rs Outdated
Comment on lines +946 to +952
"text" => {
if wecom.config.group_require_mention {
strip_bot_mention(&msg.content)
} else {
msg.content.clone()
}
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5strip_bot_mention now returns Option<String> and messages without a mention are dropped when group_require_mention is enabled.

Comment thread gateway/src/adapters/wecom.rs Outdated
Comment on lines +487 to +491
let agent_id = self.config.agent_id.parse::<u64>().unwrap_or(0);
let body = serde_json::json!({
"touser": to_user,
"msgtype": "text",
"agentid": agent_id,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5WECOM_AGENT_ID is now validated as numeric in from_env(). Invalid values disable the adapter at startup with a warning.

Comment thread docs/wecom.md Outdated
| Text message receive/reply | ✅ |
| AES-256-CBC message decryption | ✅ |
| Message deduplication | ✅ |
| Auto-split long replies (2048 chars) | ✅ |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5 — updated to "2048 bytes".

Comment thread charts/openab/templates/gateway.yaml Outdated
value: {{ ($cfg.gateway).googleChat.webhookPath | quote }}
{{- end }}
{{- end }}
{{- $hasWecom := and (($cfg.gateway).wecom).corpId (($cfg.gateway).wecom).secret (($cfg.gateway).wecom).token (($cfg.gateway).wecom).encodingAesKey }}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5agentId is now included in the $hasWecom condition in both gateway.yaml and gateway-secret.yaml. WECOM_AGENT_ID is always injected when WeCom is enabled.

Comment thread charts/openab/templates/gateway.yaml Outdated
Comment on lines +206 to +209
{{- if (($cfg.gateway).wecom).agentId }}
- name: WECOM_AGENT_ID
value: {{ ($cfg.gateway).wecom.agentId | quote }}
{{- end }}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5 — same fix as above, agentId now required in $hasWecom and always injected.

Comment thread charts/openab/values.yaml Outdated
secret: "" # App Secret → WECOM_SECRET (use --set-literal or external secret mgmt)
token: "" # Callback verification token → WECOM_TOKEN
encodingAesKey: "" # 43-char AES key → WECOM_ENCODING_AES_KEY
agentId: "" # Agent ID → WECOM_AGENT_ID (default: 0)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 22b44a5 — updated comment to "(required)" and added agentId to $hasWecom condition.

@github-actions github-actions Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 3 days label May 7, 2026
canyugs and others added 2 commits May 8, 2026 00:21
- Validate PKCS#7 padding bytes match before stripping
- Validate WECOM_AGENT_ID is numeric at startup (fail-fast)
- Gate group messages: drop when no @mention present (group_require_mention)
- Add agentId to Helm $hasWecom condition (required field)
- Fix docs: "2048 chars" → "2048 bytes"

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove standalone K8s manifest (Helm chart is the canonical way)
- Keep docker run as env var quick-start example (matches other adapters)
- Add note: group chat requires enterprise real-name verification

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
wangyuyan-agent
wangyuyan-agent previously approved these changes May 8, 2026
Copy link
Copy Markdown
Contributor

@wangyuyan-agent wangyuyan-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

PR #769 Review: feat(gateway): add WeCom (企业微信) channel adapter

1. 解決什麼問題

為 OpenAB Gateway 新增企業微信(WeCom)channel adapter,讓用戶可以透過企業微信 App 與 OAB agent 對話。涵蓋:回調驗證、AES-256-CBC 消息解密、access token 自動刷新、長消息分片、群聊 @mention 過濾、streaming reply(debounce flush)。

2. 方案是否正確

整體架構正確——遵循現有 adapter 模式(feishu / telegram / googlechat):

  • WecomConfig::from_env() 讀環境變數
  • GET 處理回調驗證、POST 處理消息回調
  • 加解密走 WeCom 官方規範(SHA1 簽名 + AES-CBC + PKCS#7)
  • Helm chart / secret / values.yaml 同步更新
  • 文檔完整(setup guide + troubleshooting)

依賴選擇合理:sha1 用於簽名驗證,quick-xml 解析 WeCom 的 XML payload,皆為成熟 crate。

3. 問題與建議

必修項(REQUEST_CHANGES):

# 問題 說明
1 缺少調研對比 PR description 未包含 Discord / OpenClaw / Hermes Agent 對 WeCom adapter 的實現方式對比。按項目規範,PR 必須附調研對比、飛書/平台 API 事實核查、設計取捨。
2 缺少設計考量與取捨 為何選 XML 解析而非 JSON 模式?streaming reply 的 debounce 策略如何決定?長消息 2048 bytes 切分依據?這些決策需要記錄。
3 Content struct 改動影響面 attachments: vec![] 被加到所有 googlechat test fixtures,說明 Content struct 新增了 attachments 欄位——但 diff 中未見 Content 定義的改動,也未見其他 adapter(teams / telegram / line / feishu)的對應更新。需確認是否所有 adapter 都已適配,或是否有 breaking change。
4 版本跳躍 0.1.0 → 0.4.0 一個 PR 直接跳三個 minor version,缺乏 CHANGELOG 說明。若中間版本已在其他 PR 發佈則無妨,但需在 PR description 交代。

建議(非阻塞):

  • WECOM_GROUP_REQUIRE_MENTION 未出現在 Helm values.yaml 中,生產部署時無法透過 Helm 配置。
  • decode_aes_key 中 with_decode_allow_trailing_bits(true) 放寬了 base64 解碼——建議加註釋說明為何需要此設定(WeCom 的 key 格式特殊性)。
  • 文檔中 "Option A: Zeabur" 作為 recommended 略顯主觀,建議改為 neutral 排列或標註 "for quick testing"。

4. Verdict

REQUEST_CHANGES

主要原因:缺少項目規範要求的調研對比與設計取捨文檔。代碼實現本身品質不錯,補齊 PR description 的三項必備內容(調研對比、API 事實核查、設計考量)後可 approve。

@shaun-agent
Copy link
Copy Markdown
Contributor

OpenAB PR Screening

This is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Click 👍 if you find this useful. Human review will be done within 24 hours. We appreciate your support and contribution 🙏

Screening report ## Intent

PR #769 adds a WeCom / 企业微信 channel adapter to the OpenAB Custom Gateway so users can talk to OAB agents from WeCom enterprise app direct messages and group chats.

The concrete operator-visible problem is that OpenAB currently lacks a first-class WeCom ingress and reply path. Teams using WeCom cannot route enterprise chat messages into agents without custom glue code, webhook handling, encryption verification, token management, and deployment configuration.

Feat

This is a feature PR.

It adds a new gateway adapter for WeCom callbacks and replies, including encrypted callback verification, XML message parsing, access token caching, message deduplication, image download/resize handling, long-response splitting, group mention gating, Helm configuration, and operator documentation.

It also appears to include a small unrelated change in gateway/src/adapters/googlechat.rs, which should be reviewed separately or justified.

Who It Serves

Primary beneficiaries: WeCom enterprise users and deployers who want OpenAB agents available inside corporate WeCom workspaces.

Secondary beneficiaries: gateway maintainers and reviewers, because the PR adds deployment docs, Helm values, and unit coverage for the new adapter.

Rewritten Prompt

Implement a WeCom channel adapter for the OpenAB Custom Gateway.

Add /webhook/wecom support that can verify WeCom callback handshakes, decrypt AES-CBC encrypted XML payloads, parse supported inbound message types, deduplicate repeated callbacks, and forward normalized gateway events to connected OAB pods. Support text DMs, group messages with configurable @mention gating, and image attachments via WeCom media download.

Add outbound reply support using WeCom’s send API, including cached access tokens, safe splitting for messages over WeCom’s 2048-byte text limit, and a streaming-compatible fallback that sends a temporary placeholder and flushes final text after debounce.

Expose configuration through documented environment variables and Helm values. Add focused tests for crypto verification, XML parsing, deduplication, group mention behavior, token refresh, message splitting, and streaming flush behavior. Keep any unrelated adapter changes out of scope unless required by a shared gateway contract.

Merge Pitch

This is worth advancing because WeCom is a major enterprise chat surface, especially for organizations operating in China, and the PR fills a clear integration gap in the gateway layer.

Risk profile: medium-high. The feature touches external crypto validation, inbound webhook security, media handling, outbound delivery, Helm secrets, and a large new adapter file. The main reviewer concern will likely be whether the implementation is too large to merge safely in one pass, whether WeCom protocol edge cases are covered, and whether the adapter follows existing gateway patterns closely enough.

Best-Practice Comparison

Relevant OpenClaw principles:

Gateway-owned scheduling is not directly relevant; this PR is event-driven webhook handling, not scheduled execution.

Durable job persistence is partially relevant. WeCom callbacks and outbound replies are handled in process, and deduplication appears TTL-based. That may be acceptable for a first channel adapter, but durable delivery state would improve reliability during gateway restarts.

Isolated executions are not central here unless media processing or streaming flush tasks can block shared gateway behavior. Review should confirm image processing is bounded and does not create unbounded memory or CPU pressure.

Explicit delivery routing is highly relevant. The adapter should clearly map WeCom users, rooms, groups, and agent replies to gateway sessions so responses cannot leak across conversations.

Retry/backoff and run logs are relevant for outbound sends, token refresh, media downloads, and placeholder recall. The PR should expose enough structured logging for operators to debug failed WeCom delivery.

Relevant Hermes Agent principles:

Gateway daemon tick model is not directly relevant because this is webhook-driven, not polling-driven.

File locking to prevent overlap is not relevant.

Atomic writes for persisted state is only relevant if token or dedupe state becomes persistent. The current in-memory approach avoids file consistency issues but loses state on restart.

Fresh session per scheduled run is not relevant.

Self-contained prompts for scheduled tasks is not relevant.

The strongest applicable principles are explicit routing, bounded isolated processing, retry/backoff, structured logs, and careful persisted-state decisions if delivery durability is later added.

Implementation Options

Conservative option: Merge a minimal WeCom text-only adapter first.

Scope this to encrypted callback verification, text DM and group message ingestion, group mention gating, access token caching, text replies, Helm/env docs, and tests. Defer images, file handling, placeholder recall, and streaming debounce to follow-up PRs.

Balanced option: Merge the current adapter after review tightening.

Keep text, group messages, image receiving, long-message splitting, token caching, deduplication, Helm support, and docs. Require review cleanup around the large wecom.rs file, error handling, logging, timeout limits, unrelated Google Chat changes, and real-world WeCom behavior notes.

Ambitious option: Introduce a more general chat-adapter delivery framework.

Use WeCom as the first implementation of a shared gateway abstraction for encrypted webhook verification, media fetch normalization, outbound delivery retries, dedupe stores, and channel-specific text splitting. This could improve future adapters but would expand the PR significantly and delay merge.

Comparison Table

Option Speed to ship Complexity Reliability Maintainability User impact Fit for OpenAB right now
Minimal text-only WeCom adapter High Low-Medium Medium High Medium Strong if reviewers want small merge steps
Current adapter with review tightening Medium Medium-High Medium-High Medium High Best balance if tests and edge cases hold
General chat-adapter framework Low High High long-term High long-term, risky short-term High later Probably too broad for this PR

Recommendation

Advance with the balanced option, but ask Masami or Pahud to review it as a security-sensitive gateway adapter rather than a routine channel addition.

Before merge discussion, split or justify the unrelated googlechat.rs change, verify WeCom crypto and callback handling against official behavior, and check that media downloads have size/time limits and clear error logs. If the PR feels too large, cut it down to text/group support first and land image receiving plus streaming polish as follow-ups.

@Joseph19820124
Copy link
Copy Markdown
Contributor

Good overall implementation — the AES-256-CBC decryption, double-checked locking on the token cache, and constant-time signature comparison are all handled correctly. 22 tests is a solid baseline. A few things worth addressing:

Bugs / correctness

group_require_mention not applied to image/file messages (wecom.rs:959-973)
Image and file messages in group chats bypass the @mention gate. A user sending an image without @-ing the bot will still trigger the agent. Probably not intended:

"image" => "Describe this image.".to_string(),  // no mention check
"file" => format!("User sent a file: {}", msg.file_name),  // no mention check

Dangling "thinking…" placeholder on debounce timeout (wecom.rs:382-390)
When the debounce task times out and last_text.is_empty(), the function returns early — but the ⏳... placeholder was already sent to the user and never recalled. The user sees a stuck placeholder forever:

Err(_) => {
    if !last_text.is_empty() { break; }
    if started.elapsed() > max_idle { break; }  // exits with empty last_text → recall skipped
}

flush_thinking should be called with an empty/error message (or the recall step split out) to clean up in this case.

strip_bot_mention only matches leading @ (wecom.rs:784-792)
WeCom group messages often have @Bot mid-sentence or at the end (e.g. "帮我查一下 @Bot"). The current implementation silently drops these, and group_require_mention=true users won't understand why the bot ignores them.

Inconsistency between code and PR description

  • Debounce timeout: code uses from_secs(3), PR description says "800ms quiet period"
  • Dedupe TTL: DEDUPE_TTL_SECS = 30 but PR description says "5min TTL"

These are likely copy-paste leftovers from an earlier draft — the code values should be updated to match the described behaviour, or vice versa.

Minor issues

WECOM_GROUP_REQUIRE_MENTION missing from Helm chart (gateway.yaml)
The env var is documented but not injected by the Helm template. Users who want false have no way to set it via values.yaml without manually patching.

Dedupe cache never purges until 10K entries (wecom.rs:180-181)
Cleanup only runs when len >= DEDUPE_MAX_SIZE. Under normal load stale 30s-old entries pile up indefinitely. A simple periodic background task or a scan on every insert would be cheaper than accumulating 10K entries first.

Tests are not thread-safe (wecom.rs:1210-1227)
std::env::set_var / remove_var in unit tests race when cargo runs tests in parallel. Either use serial_test or scope the env manipulation with a mutex.

Access token in URL (wecom.rs:249, wecom.rs:744)
WeCom's API requires access_token as a query param (so this is unavoidable), but it's worth noting in the docs/ops guide that HTTP access logs will capture live tokens — a heads-up for operators using shared proxy infrastructure.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@wangyuyan-agent wangyuyan-agent dismissed their stale review May 8, 2026 23:51

Dismissing: verdict was REQUEST_CHANGES but incorrectly posted as approve due to a bug. Apologies.

@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 9, 2026

Response to chaodu-agent third re-review (26a7b3f)

🟡 F1 — Streaming race fixed. The debounce task now acquires the pending_streams lock before reading rx.borrow(), capturing any late writes that landed between the timeout firing and entry removal. Holding the lock blocks handle_reply from sending more chunks for this channel, so the captured value is the last writeable moment.

🟡 F2 — Replay protection added. Callbacks whose timestamp is more than 5 minutes off from chrono::Utc::now() are now rejected with 403 before crypto/signature work runs. WeCom retries within ~5s, so 5min covers legitimate retries with no false positives.

🟡 F3 — Pic URL HTTPS check added. download_wecom_image rejects any non-https:// URL. Defense-in-depth against a forged callback pointing at an internal host if the AES key were ever to leak.

🟡 F4 — Helm streaming config exposed. Added streamingEnabled and debounceSecs to wecom: block in values.yaml, and conditional injection in gateway.yaml. Deployers can now set them without extraEnv overrides.


🟡 F5 — gateway CI clippy: deliberate decision, not addressing in this PR

Per commit f34a442, cargo clippy -- -D warnings was intentionally dropped from the gateway job because it surfaces ~14 pre-existing warnings across feishu.rs and googlechat.rs (collapsible_if, dead_code, manual_strip, needless_range_loop, too_many_arguments, useless_conversion, nonminimal_bool). Those warnings have nothing to do with this PR — they exist on main and would block any gateway CI extension. Fixing them belongs in its own PR (likely several per-adapter PRs given the scope), and forcing them into this PR's scope would conflate unrelated cleanup with a feature add.

The gateway job currently runs cargo check + cargo test, which is strictly more coverage than main had before this PR (zero gateway-side PR-time CI). Re-introducing clippy after the pre-existing warnings are cleaned up is the right sequencing.

CI green on 26a7b3f. Ready for re-review.

@chaodu-agent

This comment has been minimized.

Per chaodu-agent's 4th re-review:

F1: download_wecom_file used a token directly without retry. If the
cached token expired between get_token() and the GET, the file silently
failed. Added fetch_media_with_retry() which sniffs the response
Content-Type — WeCom's media API returns JSON {errcode:42001,...}
on token expiry instead of binary — and retries once after a forced
refresh. download_wecom_file now takes &WecomTokenCache and runs the
retry helper itself.

F2: TOCTOU in handle_reply's has_pending branch. The first has_pending
read happens under a lock that's then released; by the time we re-take
the lock to append, the debounce task may have removed the entry,
and we'd silently drop the chunk. Now: re-check inside the second
lock and, if the entry is gone, fall through to the direct-send path
so the chunk still reaches the user.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 9, 2026

Response to chaodu-agent 4th re-review (e836a6c)

🟡 F1 — File download token retry: fixed.

download_wecom_file no longer takes a raw token. It now takes &WecomTokenCache and delegates to a new fetch_media_with_retry() helper. The helper sniffs the response Content-Type: WeCom's /cgi-bin/media/get returns JSON {"errcode":42001,...} on token expiry instead of binary, so a JSON content-type triggers a parse, and on errcode 42001 we force_refresh and retry once. Aligns the file path with the message-send path's retry guarantee.

🟡 F2 — TOCTOU in has_pending: fixed.

Refactored the if has_pending branch so the second lock acquires under a closure that returns whether the append actually happened. If the debounce task removed the entry between the first has_pending read and the second lock, the closure returns false and we fall through to the direct-send path, so the chunk still reaches the user. Previously we'd send the response and return regardless, losing the text.

CI green. Ready for re-review.

@chaodu-agent

This comment has been minimized.

F2 (debounceSecs:0 silently ignored): document the minimum is 1 in
docs/wecom.md and values.yaml. The Helm template's truthy check treats
0 as unset; since 0-second debounce defeats the buffer purpose anyway,
documenting the floor is more honest than reshaping the truthy check
to accept a value with no real use case.

F3 (env::set_var in tests is parallel-unsafe): refactor from_env to
delegate to from_reader, which takes a closure. Tests now build a
HashMap-backed reader and never touch process-wide env vars, so
cargo's parallel runner can't race them. Also fixes the wecom
collapsible_match clippy warning while we're in there (XML parser
nested if-in-match collapsed to match guards).

F4 (no rate-limiting docs): added a "Production Hardening" section
explaining that the timestamp-freshness check rejects stale replays
cheaply but fresh-but-invalid requests still consume CPU, and pointing
to edge / LB / reverse-proxy layer for IP-level rate limits, plus the
WeCom Trusted IP allowlist as the strongest control.

F1 (clippy CI gap) and F5 (mutex poison logging) intentionally not
addressed — see the follow-up reply for rationale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 9, 2026

Response to chaodu-agent round-5 (355eca9)

Re-evaluated each finding for whether the fix actually improves the codebase versus just satisfying the reviewer. Result: 3 fixed, 2 declined with reasoning.

✅ F2 — debounceSecs: 0 falsy: documented minimum 1

The Helm template treats 0 as unset; the value 0 (= flush every chunk immediately) defeats the streaming-buffer purpose. Documenting the floor in values.yaml and docs/wecom.md is more honest than reshaping the truthy check to accept a value with no real use case. If a user later wants 0 to mean something specific, that's its own design discussion.

✅ F3 — env::set_var in tests: refactored to from_reader

Replaced both tests' env::set_var calls with a HashMap-backed reader passed to a new WecomConfig::from_reader<F: Fn(&str) -> Option<String>>. from_env now delegates to from_reader with std::env::var. Tests no longer mutate process-wide state, so cargo's parallel runner can't race them.

Bonus: cleaned up the wecom collapsible_match clippy warning (XML parser nested-if-in-match → match guards).

✅ F4 — rate-limiting docs: added "Production Hardening" section

Documents that timestamp-freshness already rejects stale replays cheaply, but fresh-but-invalid requests consume CPU, and points to edge / LB / reverse-proxy IP rate-limiting plus the WeCom Trusted IP allowlist as the strongest control.


❌ F1 — gateway CI clippy gap: not adding in this PR

This was raised in round 4 too — same answer. cargo clippy -- -D warnings on the gateway crate surfaces ~14 pre-existing warnings across feishu.rs and googlechat.rs (collapsible_if, dead_code, manual_strip, needless_range_loop, too_many_arguments, useless_conversion, nonminimal_bool). I don't have the domain knowledge to know whether each dead_code field is truly dead vs. reserved for forward compat — guessing comments would be worse than the warnings. The right path is per-adapter cleanup PRs owned by their respective maintainers, not WeCom-feature-PR scope creep.

The gateway job currently runs cargo check + cargo test — strictly more coverage than main had before this PR. Re-introducing clippy belongs after the cleanup PRs land.

❌ F5 — mutex poison silent recovery: declining the suggested fix

lock().unwrap_or_else(|e| e.into_inner()) is correct for availability. The proposed tracing::warn! on every poison call would be noisy: mutex poison is per-mutex persistent state, so once poisoned, every subsequent lock would log a warning until the mutex is replaced — a flood pattern that hides the original poison cause.

Doing this right would need stateful "first-encounter only" tracking, which is non-trivial. Or panic instead of recover, which sacrifices availability for clarity. Neither is clearly better than the current code, so leaving as-is.


CI green on 355eca9. Ready for re-review.

canyugs and others added 3 commits May 9, 2026 09:19
Restores the strict-clippy parity that the root crate has and that
reviewers (chaodu round 4 and 5) flagged as a CI consistency gap.
The mechanical fixes don't change behavior:

- dead_code on serde-deserialize fields → #[allow(dead_code)] with
  fact-only "parsed by serde, not consumed in current code paths"
  (no speculation about future intent)
- needless_range_loop in markdown rendering → buf.extend(slice.iter())
- manual_strip in fenced code block detection → strip_prefix
- useless_conversion on tungstenite Message::Binary → drop the .into()
- too_many_arguments on ws_connect_loop / handle_ws_message →
  #[allow(clippy::too_many_arguments)]; refactoring 11-arg async fn
  signatures is a larger change that doesn't belong here

Any further warnings exposed by this strict job will be visible in
the CI run and addressed in a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After merging upstream main and enabling strict clippy on the gateway
job, 5 more warnings surfaced:

- feishu.rs: collapsible_if — flatten outer + !(in_thread &&
  bypass_mention_gating) into one if condition
- feishu.rs: nonminimal_bool — !is_some_and(<) → is_none_or(>=)
- wecom.rs: too_many_arguments on flush_thinking (8 args) →
  #[allow(clippy::too_many_arguments)]
- wecom.rs: useless_vec on parts vec → use 4-element array, sort_unstable
- main.rs: explicit_auto_deref on &*text → &text

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chaodu-agent

This comment has been minimized.

…wn limits)

F1 (corpsecret in URL): WeCom's gettoken API requires the secret as a
query param; we cannot move it to a header. Added a comment in code
clarifying the protocol constraint and a "Redact corpsecret from
access logs" section in docs/wecom.md instructing operators to redact
query strings at the proxy layer for /cgi-bin/gettoken outbound calls.

F2 (byte-vs-char comment): added a doc comment to split_text_lines
making explicit that the limit and all len() comparisons are in
bytes (matching WeCom's server-side truncation), and that lines
exceeding the limit are split at UTF-8 char boundaries.

F3 (streaming task lifetime on shutdown): documented as known
limitation. The fix would add a JoinSet/CancellationToken on the
adapter; non-trivial scope, and impact is bounded since streaming
defaults off. Recorded in the new "Known limitations" docs section.

F4 (DedupeCache eviction is lazy): unchanged, documented under known
limitations. ~500 KB max memory bound is acceptable; correctness
(dedup window honored) is unaffected. Repeated finding from
Copilot/chaodu earlier rounds; canyugs's prior "won't fix" rationale
still applies.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@canyugs
Copy link
Copy Markdown
Contributor Author

canyugs commented May 9, 2026

Response to chaodu-agent round-6 (7d83332)

Plus important: F1 from round 4/5 (gateway clippy gap) is now actually fixed end-to-end — see a58e1ba + 8b55672 for the full pre-existing-warning cleanup, and the new gateway CI job in .github/workflows/ci.yml runs cargo clippy -- -D warnings matching the root crate's strict standard.

✅ F1 (round 6) — corpsecret in gettoken URL: comment + docs

WeCom's protocol requires corpsecret as a query parameter (no header alternative). The gateway itself never logs this URL. Added an inline code comment at the call site and a new docs section "Redact corpsecret from access logs" instructing operators to configure proxy log sanitization for /cgi-bin/gettoken outbound calls.

✅ F2 — byte-vs-char clarification: doc comment on split_text_lines

You correctly flagged in your own writeup that the implementation is consistent (byte-based throughout). Added a doc comment making this explicit: limit is in bytes (matching WeCom's server-side truncation), char-boundary splitting only kicks in when a single line exceeds the byte limit.

✅ F3 — debounce task lifetime on shutdown: documented known limitation

A proper fix needs a JoinSet/CancellationToken on the adapter, which is a non-trivial refactor. Impact is bounded — streaming defaults off, and on SIGTERM the runtime drops the spawned task; in-flight buffered text is lost but the agent typically re-emits on the next interaction. Recorded under new "Known limitations" in docs/wecom.md for operator awareness; tracked as follow-up.

❌ F4 — DedupeCache lazy eviction: keeping current behavior, documented

This is the same finding Copilot raised in round 1 (issue #2) where canyugs marked it "won't fix" with rationale: WeCom enterprise apps don't see the message rates that would benefit from periodic eviction. Below DEDUPE_MAX_SIZE the HashMap can hold stale entries, but max memory is bounded (~500 KB), and correctness (the 30s dedup window) is honored. Adding a periodic sweep adds a background task + complexity for negligible benefit. Documented in "Known limitations" so the design choice is visible.


CI green on 7d83332, including strict clippy. Ready for re-review.

@chaodu-agent
Copy link
Copy Markdown
Collaborator

LGTM ✅ — Well-engineered WeCom adapter that correctly implements the platform crypto spec, follows existing adapter patterns, and ships with thorough tests and documentation.

What This PR Does

Adds WeCom (企业微信) enterprise app support to the Custom Gateway, enabling 1:1 direct message conversations between WeCom users and OAB agents via the self-built app callback API.

How It Works

The adapter handles the full WeCom callback lifecycle: AES-256-CBC message decryption (PKCS7 block_size=32, IV=key[..16]), SHA1 signature verification (constant-time via subtle), access token caching with double-checked locking and auto-refresh on 42001, message deduplication (30s TTL, 10K cap), image download with resize/compress, text file detection, and optional streaming via thinking-placeholder + debounce + recall. Routes are registered as GET (verify) and POST (webhook) on the configurable path. Helm templates inject secrets via secretKeyRef and plain values for non-sensitive config.

Findings

# Severity Finding Location
1 🟢 Correct WeCom crypto implementation — PKCS7/32, NoPadding decrypt + manual strip, IV=key[..16] wecom.rs:130-175
2 🟢 Constant-time signature comparison prevents timing attacks wecom.rs:107
3 🟢 HTTPS-only image download prevents SSRF via forged PicUrl wecom.rs:1105
4 🟢 5-minute timestamp freshness check rejects replay attacks wecom.rs:webhook
5 🟢 Streaming defaults to off, avoiding UX flicker for new deployments wecom.rs:44
6 🟢 Token retry on 42001 covers long-running streams that outlive cached token wecom.rs:post_with_token_retry
7 🟢 Secrets stored in K8s Secret via secretKeyRef, not plain env gateway.yaml
8 🟢 21 unit tests including wiremock integration for token refresh wecom.rs:tests
9 🟢 Comprehensive operator docs with production hardening guidance docs/wecom.md
10 🟢 CI now covers the gateway crate (was previously skipped) ci.yml
Finding Details

🟢 F1: Correct WeCom crypto implementation

The adapter correctly handles WeCom's non-standard PKCS7 padding (block_size=32 instead of AES's native 16). It decrypts with NoPadding and manually validates/strips the padding, which is the correct approach. The EncodingAESKey base64 decode with allow_trailing_bits handles WeCom's 43-char key format properly.

🟢 F2: Constant-time signature comparison

Uses subtle::ConstantTimeEq for signature verification, preventing timing side-channel attacks on the HMAC comparison.

🟢 F3: SSRF protection on image download

Rejects non-HTTPS pic_url values, preventing an attacker who compromises the AES key from using forged callbacks to probe internal network hosts.

🟢 F4: Replay protection

The 5-minute timestamp freshness check ((now - ts).abs() > 300) rejects stale callbacks before any crypto runs, limiting the replay window even if an attacker captures a valid signed payload.

🟢 F5: Streaming defaults off

The thinking-placeholder + recall pattern causes a brief client flicker. Defaulting to off is the right UX choice; operators can opt in with WECOM_STREAMING_ENABLED=true.

🟢 F6: Token retry on 42001

Both send_text and flush_thinking go through post_with_token_retry, ensuring a long-running debounce stream that outlives the 7200s token TTL still delivers its final reply.

🟢 F7: Secrets in K8s Secret

WECOM_SECRET, WECOM_TOKEN, and WECOM_ENCODING_AES_KEY are injected via secretKeyRef in the Helm template, while non-sensitive CORP_ID and AGENT_ID are plain values. Correct separation.

🟢 F8: Comprehensive test coverage

21 tests covering: config parsing, AES key decode, signature verify/reject, encrypt-decrypt roundtrip, XML parsing (text/image/file), dedup cache, token refresh with wiremock, text splitting (including UTF-8 char boundaries), text file detection, and full webhook flow simulation.

🟢 F9: Production hardening docs

docs/wecom.md includes rate limiting guidance, corpsecret log redaction advice, streaming task lifetime on shutdown, and dedup cache memory bounds. This is above-average for a new adapter PR.

🟢 F10: CI coverage for gateway

Adding a dedicated gateway job with cargo check + cargo clippy + cargo test ensures the gateway crate (previously untested in CI) is now covered.

Baseline Check
  • PR opened: 2026-05-07
  • Main already has: No WeCom code whatsoever
  • Net-new value: Complete WeCom channel adapter (1654 lines), Helm integration, CI coverage, operator docs
What's Good (🟢)
  • Follows existing adapter patterns (feishu, googlechat) — consistent architecture
  • Prior art research (OpenClaw, Hermes Agent) documented in PR body with comparison table
  • Alternatives considered section explains why in-tree, why callback mode, why inline crypto
  • Mutex poison recovery (unwrap_or_else(|e| e.into_inner())) keeps the server up
  • ToUserName envelope check before crypto prevents wasted CPU on misrouted callbacks
  • split_text_lines handles UTF-8 char boundaries correctly (no mid-codepoint truncation)
  • Debounce task has a 5-minute max idle timeout preventing leaked tasks
  • The PR body is exceptionally well-written with clear scope boundaries and follow-up tracking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pending-contributor pending-screening PR awaiting automated screening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support WeCom (企業微信) as a new channel adapter in openab/gateway

7 participants