feat(channels): add WeCom intelligent robot channel#6224
Conversation
|
Re-triage at Template still good ✓ Problem and direction unchanged — real user need (#6208), aligned with the existing IM channel adapter pattern. Delta since
All previously-flaged concerns now have targeted fixes. Moving to code review. 🔍 中文说明在 模板完整 ✓ 问题和方向不变——真实用户需求（#6208），与现有 IM 频道适配器模式一致。 新增改动：ChannelBase 同步 cancel 修复、SSRF 修补（SIIT IPv4 映射 + IPv6 文档段）、合并附件清理、SDK connect 超时限制、按路由隔离未跟踪附件清理。 所有之前标记的问题都有了针对性修复。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code Review —
|
| Suite | Result |
|---|---|
| ChannelBase (284 tests) | ✅ All pass |
| WeComAdapter (90 tests) | ✅ All pass |
| config-utils (29 tests) | ✅ All pass |
| runtime (4 tests) | ✅ All pass |
| typecheck | ✅ Clean |
| build | ✅ Clean |
中文说明
代码审查 — 8ab3343 → 40ca360（6 个提交）
ChannelBase cancel 回归修复 (05936c1)： 干净最小化修复。preflightInbound 返回 boolean | Promise<boolean>，同步路径保持无微任务。284 个 ChannelBase 测试全部通过。
SSRF 修补 (d9a9b84)： 新增 SIIT IPv4 映射 IPv6 和 IPv6 文档段拦截。连接处理改为 Promise.all，防止未处理 rejection。
SDK connect 超时 (c3350a1)： 30 秒超时限制，timer 使用 unref，测试验证超时触发。
附件清理隔离 (6e25684 + 40ca360)： 从扁平数组改为按路由键索引的 Map，防止跨会话误删。并发 alice/bob 测试验证了隔离效果。
无新发现问题。 Delta 专注，每个提交直接针对之前标记的问题。
真实场景测试
用无效凭证运行 qwen channel start test-wecom：连接真实 WeCom API，认证失败报告清晰，无崩溃，进程干净退出。
全部测试套件通过：ChannelBase 284、WeCom 90、config-utils 29、runtime 4、typecheck 和 build 均正常。
— Qwen Code · qwen3.7-max
|
Stepping back: the PR's structure and approach are sound — it follows the established channel adapter pattern, the test coverage is reasonable, and the scope is appropriate for a new channel integration. If I were writing this from scratch, I'd take essentially the same approach: new package under But four real issues stand between this and merge. The SDK logger leak is the most urgent — it's a data privacy concern in production that's already visible in our test output (the These are all fixable with targeted changes — none require rearchitecting. The foundation is good. Once @qqqys addresses these four items, this should be ready to ship. Not approving or rejecting — deferring to @wenshao who already flagged these in the inline review and can confirm when the fixes land. 中文说明PR 整体结构和方案合理，遵循了现有的频道适配器模式，范围适当。如果从零开始写，我也会采取同样的方式。 但有四个真实问题需要解决后才能合并：SDK 日志泄漏（生产环境数据隐私风险）、 这些都是可以针对性修复的问题，不需要重构。基础是好的。不批准也不拒绝——交给已经指出这些问题的 @wenshao，等修复后确认即可。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| text: extractText(body), | ||
| messageId, | ||
| isGroup, | ||
| isMentioned: true, |
There was a problem hiding this comment.
[Suggestion] isMentioned is hardcoded to true for every inbound message. This silently bypasses the requireMention group policy documented in wecom.md — the GroupGate in ChannelBase drops messages only when requireMention && !isMentioned && !isReplyToBot, so every group message will reach the agent regardless of whether the bot was actually mentioned.
Other adapters compute this dynamically: DingTalk checks isInAtList, Feishu scans the mentions array, Telegram checks for @botname in text.
If the WeCom intelligent robot SDK only delivers messages where the bot was @-mentioned (which is likely for smart bots in API mode), this is technically correct — but the assumption should be documented. If not, consider parsing the actual mention data from the SDK payload, or setting isMentioned: !isGroup as a conservative default for groups.
— qwen3.7-max via Qwen Code /review
| const refs = collectInboundMediaRefs(body); | ||
| const attachments: Attachment[] = []; | ||
| for (const ref of refs) { | ||
| const downloaded = await client.downloadFile(ref.url, ref.aesKey); |
There was a problem hiding this comment.
[Suggestion] Inbound attachments are downloaded entirely into memory (downloaded.buffer) with no size check. Outbound media has MAX_OUTBOUND_MEDIA_BYTES (20 MB), but inbound has no equivalent. The Feishu adapter enforces MAX_DOWNLOAD_BYTES = 50 * 1024 * 1024 via Content-Length header check and streaming read with actual size enforcement (packages/channels/feishu/src/media.ts).
A message referencing a very large resource (or multiple large resources in a mixed message) could exhaust memory. Consider adding a size cap after download:
| const downloaded = await client.downloadFile(ref.url, ref.aesKey); | |
| const downloaded = await client.downloadFile(ref.url, ref.aesKey); | |
| const data = downloaded.buffer; | |
| if (data.length > MAX_OUTBOUND_MEDIA_BYTES) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] skipping oversized attachment (${data.length} bytes): ${ref.url}\n`, | |
| ); | |
| continue; | |
| } |
— qwen3.7-max via Qwen Code /review
| add('file', getRecord(body, 'file') ?? {}); | ||
| add('video', getRecord(body, 'video') ?? {}); | ||
|
|
||
| const quote = getRecord(body, 'quote'); |
There was a problem hiding this comment.
[Suggestion] collectInboundMediaRefs recurses into body.quote without a depth limit. While WeCom realistically sends at most one level of quote nesting, a malformed or unexpected payload with deeply nested quotes would cause a stack overflow, crashing the message handler.
Consider adding a depth parameter:
| const quote = getRecord(body, 'quote'); | |
| function collectInboundMediaRefs( | |
| body: Record<string, unknown>, | |
| depth: number = 0, | |
| ): InboundMediaRef[] { | |
| if (depth > 3) return []; |
And pass depth + 1 in the recursive call:
| const quote = getRecord(body, 'quote'); | |
| if (quote) refs.push(...collectInboundMediaRefs(quote, depth + 1)); |
— qwen3.7-max via Qwen Code /review
| const allowedDirs = [ | ||
| realpathSync(tmpdir()), | ||
| realpathSync(resolve(cwd)), | ||
| realpathSync('/tmp/'), |
There was a problem hiding this comment.
[Suggestion] realpathSync('/tmp/') will throw ENOENT on Windows or any system where /tmp/ does not exist, crashing sendMessage before the path check even runs. On Linux, tmpdir() already returns /tmp, making this entry redundant.
Note: the same pattern exists in packages/channels/weixin/src/send.ts, so this is a pre-existing codebase issue. But since this is new code, consider wrapping it defensively:
| realpathSync('/tmp/'), | |
| const allowedDirs = [ | |
| realpathSync(tmpdir()), | |
| realpathSync(resolve(cwd)), | |
| ]; | |
| try { allowedDirs.push(realpathSync('/tmp/')); } catch { /* /tmp not available */ } |
— qwen3.7-max via Qwen Code /review
| options.wsUrl = this.wecom.wsUrl; | ||
| } | ||
|
|
||
| const client = new ClientCtor(options); |
There was a problem hiding this comment.
[Critical] This leaves the SDK's default logger enabled. The installed SDK's WebSocket manager logs callback frames with body=${JSON.stringify(frame.body)} via console.debug; those bodies include user message text, media URLs, and aeskeys before this adapter's sanitized stderr logging runs. Please pass a channel logger to WSClient that drops frame-body debug logs and sanitizes/truncates warnings/errors.
— GPT-5 via Qwen Code /review
|
|
||
| for (const item of media) { | ||
| const file = readOutboundMedia(item.path, this.config.cwd); | ||
| const upload = await this.client.uploadMedia(file.data, { |
There was a problem hiding this comment.
[Critical] The real SDK expects uploadMedia() options to contain type, not mediaType. Its implementation destructures const { type, filename } = options, so this sends type: undefined to upload init and the documented [IMAGE:]/[FILE:] path can fail even though the mock test passes.
Use the SDK shape here and align the local mock/interface with it:
await this.client.uploadMedia(file.data, {
type: item.type,
filename: file.fileName,
});— GPT-5 via Qwen Code /review
| throw new Error(`Media file too large: ${stat.size} bytes`); | ||
| } | ||
|
|
||
| const allowedDirs = [ |
There was a problem hiding this comment.
[Critical] This allowlist is applied to every outbound media marker, including [FILE:]. Because markers are model output, a channel user can prompt the model to emit [FILE: /workspace/.env] or another file under cwd/temp, and the adapter will read and upload it directly to WeCom without going through tool approval/audit. Please restrict uploads to a channel-owned/generated export directory or otherwise prove the file was intentionally produced for this turn; at minimum, don't allow generic file/video/voice markers to read arbitrary workspace files.
— GPT-5 via Qwen Code /review
| if (typeof value !== 'string') { | ||
| throw new Error(`Channel "${name}" field "${field}" must be a string.`); | ||
| } | ||
| resolvedRawConfig[field] = resolveEnvVars(value); |
There was a problem hiding this comment.
[Critical] Required fields that are also known credentials are now env-expanded twice. For telegram.token and dingtalk/feishu client credentials, the required-field loop writes the resolved value into resolvedRawConfig, and resolveOptionalStringField() calls resolveEnvVars() again here. A valid secret whose actual value starts with $ will be treated as a second env reference and can fail startup. Resolve each credential once, e.g. compute token/clientId/clientSecret from rawConfig and let the final return override the spread values.
— GPT-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
| function extractText(body: Record<string, unknown>): string { | ||
| const msgType = getString(body, 'msgtype'); | ||
| if (msgType === 'mixed') { | ||
| const mixed = getRecord(body, 'mixed'); |
There was a problem hiding this comment.
[Suggestion] The msg_item loop only keeps entries whose msgtype is "text", so voice-transcription text carried inside a mixed message (WeCom can deliver a voice item with its transcription alongside text and image items) is silently dropped. The PR description explicitly lists "voice transcription" as one of the inbound types this channel normalizes, so this looks like an unintended gap rather than a deliberate design choice.
Consider also pulling the transcribed content out of voice items inside msg_item:
| const mixed = getRecord(body, 'mixed'); | |
| return items | |
| .map((item) => { | |
| const record = asRecord(item); | |
| if (!record) return ''; | |
| const itemType = getString(record, 'msgtype'); | |
| if (itemType === 'text') { | |
| return getString(getRecord(record, 'text'), 'content'); | |
| } | |
| if (itemType === 'voice') { | |
| return getString(getRecord(record, 'voice'), 'content'); | |
| } | |
| return ''; | |
| }) | |
| .filter(Boolean) | |
| .join('\n') | |
| .trim(); |
Adding a message.mixed test case with a voice item would also pin the behavior.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
🔬 Maintainer verification — local real build @
|
| # | Finding | Status vs real SDK | Sev |
|---|---|---|---|
| 1 | SDK default logger prints raw inbound frame.body (user text, media URL, aeskey) via console.debug |
CONFIRMED | High |
| 2 | uploadMedia({ mediaType }) → real SDK reads { type } → type: undefined on the wire |
CONFIRMED | High |
| 3 | Model-emitted [FILE:]/[VIDEO:]/[VOICE:] reads any file under cwd/tmp and pushes it to the chat, no tool approval |
CONFIRMED | High |
| 4 | config-utils double env-expansion — regression for telegram/dingtalk/feishu creds |
CONFIRMED | Med |
Evidence per finding
#1 — logger leak. connect() (WeComAdapter.ts:91‑99) builds new WSClient({ botId, secret }) with no logger, so the SDK's DefaultLogger stays on. WsConnectionManager.handleFrame runs logger.debug('[server -> plugin] … body=' + JSON.stringify(frame.body)), and DefaultLogger.debug → console.debug with no level gate (Node always emits). One injected callback frame leaked the user's message text, the media download URL and the aeskey verbatim — defeating the adapter's own sanitizeLogText(…, 200) discipline. This also survives the daemon worker-log redactor (it targets credential‑shaped tokens / sensitive env values, not free‑form body text). Fix: pass a channel logger to WSClient that drops frame‑body debug and sanitizes warn/error.
#2 — type vs mediaType. SDK impl (index.esm.js:1168): const { type, filename } = options. The adapter passes { mediaType: item.type, filename } (WeComAdapter.ts:153‑156, and the local WeComClient interface + the test both mirror the wrong key). Driving the real uploadMedia, the upload‑init frame becomes {"filename":"out.png"} — type is dropped — and the SDK's own INFO log prints type=undefined. So all outbound media ([IMAGE]/[FILE]/[VOICE]/[VIDEO]) is malformed against a real tenant, while the mock test is green. Fix: send { type: item.type, filename }, and align the interface + test.
#3 — arbitrary file upload. Markers are model output. Feeding the compiled adapter a turn of …[FILE: <cwd>/.env], readOutboundMedia accepted the path (it lives under cwd) and handed OPENAI_API_KEY=sk-… / DB_PASSWORD=… straight to uploadMedia → it would be posted to the WeCom chat, bypassing the tool‑approval/audit path a normal read_file would take. Realistic under prompt‑injection or a tool result that echoes a path. Fix: restrict uploads to a channel‑owned generated‑exports dir, or prove the file was produced this turn; don't let generic file/video/voice markers read arbitrary workspace files.
#4 — double env-expansion (regression). The PR's config-utils change adds resolvedRawConfig[field] = resolveEnvVars(value) in the required‑field loop and switches credential resolution to read resolvedRawConfig, so token/clientId/clientSecret (which are also required for telegram/dingtalk/feishu) get resolveEnvVars()'d twice. An env‑referenced secret whose resolved value starts with $ now throws at startup (pre‑PR it worked). A differential test fails on head and passes after a 3‑line fix, while the PR's own 25 tests stay green:
// config-utils.ts — resolve the credential trio ONCE, from rawConfig:
const token = resolveOptionalStringField(name, rawConfig, 'token') ?? '';
const clientId = resolveOptionalStringField(name, rawConfig, 'clientId');
const clientSecret = resolveOptionalStringField(name, rawConfig, 'clientSecret');Notes (non-blocking)
- ✅ The "harden" commit correctly addressed 4 of the earlier ci-bot suggestions (mention parsing, inbound size cap, quote‑depth guard,
safeRealpath). ⚠️ Minor: voice transcription carried inside amixedmessage is dropped —extractTextonly keepsmsgtype === 'text'items inmsg_item, contradicting the "voice transcription" claim for mixed messages.- ℹ️ The branch is behind
main(merge‑base2126474); a 2‑dotpackage.jsondiff looks like it revertsprepare/test:release/preflight, but the true net diff only adds thewecomworkspace line — GitHub's Files tab (3‑dot) is clean. A rebase would remove the confusion.
Recommendation
Nice feature and the structure is solid, but request changes: #2 means the media feature doesn't actually work against a real WeCom tenant (and its test hides that), #1 leaks user secrets to logs by default, and #3 is an unapproved exfiltration path. All four are small, well‑scoped fixes — happy to re‑verify once pushed.
🀄 中文版（点击展开）
🔬 维护者验证 —— 本地真实构建 @ f32f098ce
我在独立 worktree 里真实构建了本 PR 的 head（npm ci + tsc --build），并使用真实的 @wecom/aibot-node-sdk@1.0.7（非 mock）通过 Node 脚本 + vitest 驱动编译后的适配器，用运行时证据复现了我之前提出的 4 个 [Critical]。head 自我评审后未移动（f32f098ce 是最后一次提交，我的评审晚 5 分钟）。
基线： PR 自带用例全绿 —— WeComAdapter.test.ts 10/10、config-utils.test.ts 25/25。一个对照变异（关掉入站大小上限）恰好只翻红「大小上限」那一条，说明测试确实在跑真实代码；下述 bug 之所以能通过，是因为对应路径没有测试覆盖（而且 #2 的测试把错误的字段契约钉死了）。
结论
| # | 问题 | 对真实 SDK 的状态 | 严重度 |
|---|---|---|---|
| 1 | SDK 默认 logger 通过 console.debug 打印原始入站 frame.body（用户文本、媒体 URL、aeskey） |
已确认 | 高 |
| 2 | uploadMedia({ mediaType }) → 真 SDK 读的是 { type } → 线上 type: undefined |
已确认 | 高 |
| 3 | 模型输出的 [FILE:]/[VIDEO:]/[VOICE:] 会读取 cwd/tmp 下任意文件并发到会话，无工具审批 |
已确认 | 高 |
| 4 | config-utils 二次环境变量展开 —— telegram/dingtalk/feishu 凭证回归 |
已确认 | 中 |
（截图见上方英文部分）
各项证据
#1 —— 日志泄漏。 connect()（WeComAdapter.ts:91‑99）用 new WSClient({ botId, secret }) 构造，未传 logger，因此 SDK 的 DefaultLogger 生效。WsConnectionManager.handleFrame 会执行 logger.debug('[server -> plugin] … body=' + JSON.stringify(frame.body))，而 DefaultLogger.debug → console.debug 没有级别门控（Node 一律输出）。注入一个回调帧后，用户消息文本、媒体下载 URL 与 aeskey 被原样打印，绕过了适配器自身在别处用的 sanitizeLogText(…, 200)。该泄漏也能绕过 daemon worker 日志脱敏（脱敏只针对凭证形状的 token / 敏感 env 值，不处理自由文本 body）。建议：给 WSClient 传自定义 logger，丢弃 frame‑body 的 debug，并对 warn/error 脱敏截断。
#2 —— type vs mediaType。 SDK 实现（index.esm.js:1168）：const { type, filename } = options。适配器传的是 { mediaType: item.type, filename }（WeComAdapter.ts:153‑156，且本地 WeComClient 接口和测试都跟着用了错误字段）。用真实 uploadMedia 驱动，上传初始化帧变成 {"filename":"out.png"} —— type 丢失 —— SDK 自己的 INFO 日志也打印 type=undefined。因此对真实租户来说，所有出站媒体（[IMAGE]/[FILE]/[VOICE]/[VIDEO]）都是畸形请求，而 mock 测试却是绿的。建议：改传 { type: item.type, filename }，并对齐接口和测试。
#3 —— 任意文件上传。 标记来自模型输出。给编译后的适配器喂一段 …[FILE: <cwd>/.env]，readOutboundMedia 因该路径位于 cwd 内而放行，把 OPENAI_API_KEY=sk-… / DB_PASSWORD=… 直接交给 uploadMedia → 会被发到 WeCom 会话，绕过了正常 read_file 应走的工具审批/审计。在提示注入或工具结果回显路径时是现实可达的。建议：只允许上传频道自有的「生成导出目录」，或证明文件是本轮产生的；不要让通用的 file/video/voice 标记读取工作区任意文件。
#4 —— 二次环境变量展开（回归）。 本 PR 的 config-utils 改动在必填字段循环里加了 resolvedRawConfig[field] = resolveEnvVars(value)，同时把凭证解析改为读 resolvedRawConfig，于是 token/clientId/clientSecret（对 telegram/dingtalk/feishu 而言同样是必填）被 resolveEnvVars() 两次。当一个以 env 引用、且其真实值以 $ 开头的密钥时，现在会在启动时抛错（改动前是正常的）。差分测试在 head 上失败、打上 3 行修复后通过，而 PR 自带的 25 个测试仍然全绿（见上方第二张截图）：
// config-utils.ts —— 凭证三件套只解析一次，从 rawConfig 读：
const token = resolveOptionalStringField(name, rawConfig, 'token') ?? '';
const clientId = resolveOptionalStringField(name, rawConfig, 'clientId');
const clientSecret = resolveOptionalStringField(name, rawConfig, 'clientSecret');说明（不阻塞）
- ✅ 「harden」提交已正确处理了之前 ci-bot 的 4 条建议（提及解析、入站大小上限、引用递归深度、
safeRealpath）。 ⚠️ 小问题：mixed消息内嵌的语音转写会被丢弃 ——extractText在msg_item里只保留msgtype === 'text'，与描述中「语音转写」的说法在 mixed 场景下不符。- ℹ️ 分支落后于
main（merge‑base2126474）；2‑dot 的package.jsondiff 看起来像回退了prepare/test:release/preflight，但真实净 diff 只新增了wecom这一行 workspace —— GitHub 的 Files 页（3‑dot）是干净的。rebase 一下可消除误解。
建议
功能不错、结构也扎实，但建议 request changes：#2 意味着媒体功能对真实 WeCom 租户根本不工作（且其测试把问题掩盖了），#1 默认把用户密钥泄漏到日志，#3 是一条无审批的外泄路径。四个问题都是小而聚焦的修复 —— 推上来后我可以复验。
Verified by the maintainer via local worktree build (real SDK, no live tenant) + tmux. Harnesses: real WSClient frame injection (#1), real uploadMedia init-frame capture (#2), compiled-dist adapter drive (#3), real parseChannelConfig differential (#4).
| rawPath: string, | ||
| cwd: string, | ||
| ): { data: Buffer; fileName: string } { | ||
| const resolved = resolve(rawPath); |
There was a problem hiding this comment.
[Critical] resolve(rawPath) resolves relative paths against process.cwd(), not the channel's configured cwd. In daemon mode where the process CWD is typically / or a service directory, a media marker like [IMAGE: output.png] will resolve to the wrong file or get rejected by the allowlist even though the file exists inside config.cwd.
| const resolved = resolve(rawPath); | |
| const resolved = resolve(cwd, rawPath); |
— qwen3.7-max via Qwen Code /review
| msgtype: 'markdown', | ||
| markdown: { content: cleanedText }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Critical] The entire response is sent as a single markdown message with no length splitting. WeCom's markdown API has a ~4096-byte limit. Both DingTalk (splitChunks at ~3800 chars) and Feishu (splitChunks at ~4000 chars) split long responses into multiple chunks. Long assistant responses will be silently truncated or rejected by the WeCom API.
Consider extracting or reusing a splitChunks helper (see packages/channels/feishu/src/markdown.ts:208 or packages/channels/dingtalk/src/markdown.ts:84) and iterating over chunks in sendMessage.
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| private async onMessage(payload: unknown): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] throw on media upload failure aborts the entire media loop, dropping all subsequent media items in the same response. DingTalk and Feishu log failures and continue. A single transient upload error (network blip) prevents any remaining images from being sent.
| private async onMessage(payload: unknown): Promise<void> { | |
| if (!mediaId) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] upload returned no media_id, skipping.\n`, | |
| ); | |
| continue; | |
| } |
— qwen3.7-max via Qwen Code /review
| }); | ||
| } | ||
|
|
||
| for (const item of media) { |
There was a problem hiding this comment.
[Suggestion] Raw agent markdown is sent via msgtype: 'markdown' without platform-specific normalization. WeCom's markdown is a strict subset — it does not support headers (#), tables, horizontal rules, or fenced code blocks. DingTalk has normalizeDingTalkMarkdown() for the same reason. Users will see ## Summary as literal text, code blocks as raw backtick-fenced text, and tables as unreadable pipe-separated lines.
Consider adding a WeCom markdown normalization step, or using msgtype: 'text' for responses that don't use WeCom-supported features.
— qwen3.7-max via Qwen Code /review
| @@ -0,0 +1,407 @@ | |||
| import { mkdtempSync, writeFileSync } from 'node:fs'; | |||
There was a problem hiding this comment.
[Suggestion] Several important code paths lack test coverage:
disconnect()lifecycle — clearingdedupTimer, emptyingseenMessages, callingclient.disconnect(), and nulling the client reference are all untested. A reconnect-after-disconnect would silently fail.- Message deduplication — same
msgidarriving twice should be silently dropped; no test verifies this. - Error recovery rollback — the catch block removes
messageIdfromseenMessageswhenhandleInboundthrows; this rollback is untested. - Path-traversal rejection — the
allowedDirssecurity boundary (files outside tmpdir/cwd/tmp are rejected) has no negative test.
— qwen3.7-max via Qwen Code /review
| if (itemType === 'image') add('image', getRecord(record, 'image') ?? {}); | ||
| } | ||
|
|
||
| add('image', getRecord(body, 'image') ?? {}); |
There was a problem hiding this comment.
[Suggestion] collectInboundMediaRefs never collects voice attachments at the top level — add('voice', ...) is missing despite the adapter subscribing to message.voice events, having explicit voice-to-audio type mapping in downloadAttachments (line 262), and mediaTypeToMime handling voice with audio/amr. Voice audio files are never downloaded. For voice messages without a transcription, the agent receives only (voice) text with no audio data.
The mixed-message msg_item loop also only collects image items, silently dropping file, voice, and video items within mixed messages.
| add('image', getRecord(body, 'image') ?? {}); | |
| add('image', getRecord(body, 'image') ?? {}); | |
| add('file', getRecord(body, 'file') ?? {}); | |
| add('video', getRecord(body, 'video') ?? {}); | |
| add('voice', getRecord(body, 'voice') ?? {}); |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Two findings from this review (mixed msg_item only handling image type, and resolve(rawPath) using process.cwd() instead of config cwd) overlap with comments posted on prior commits — skipping to avoid duplicates.
Test coverage gaps still worth addressing (not previously flagged in detail):
- Dedup mechanism — no test emits two payloads with the same
msgidto verify the second is dropped disconnect()cleanup — timer cleanup,client.disconnect(),seenMessages.clear()unverified- Path traversal boundary — no negative test for
readOutboundMediawith a path outside allowed dirs onMessagecatch rollback — no test triggershandleInboundfailure to verifyseenMessages.deletedetectImageMimeformats — only PNG magic bytes tested; GIF, WEBP, JPEG, fallback untested
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| async sendMessage(chatId: string, text: string): Promise<void> { | ||
| if (!this.client) { |
There was a problem hiding this comment.
[Suggestion] sendMessage accesses this.client across multiple await points without capturing a local reference. If disconnect() runs between awaits (e.g., during graceful shutdown), this.client becomes undefined and the next property access throws a TypeError. The sibling downloadAttachments method (line 230) correctly captures const client = this.client — sendMessage should follow the same pattern.
| if (!this.client) { | |
| async sendMessage(chatId: string, text: string): Promise<void> { | |
| const client = this.client; | |
| if (!client) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] No active SDK client, cannot send.\n`, | |
| ); | |
| return; | |
| } | |
| const { cleanedText, media } = parseOutboundMediaMarkers(text); | |
| if (cleanedText) { | |
| await client.sendMessage(chatId, { | |
| msgtype: 'markdown', | |
| markdown: { content: cleanedText }, | |
| }); | |
| } | |
| for (const item of media) { | |
| if (item.type !== 'image') { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] skipping unsupported outbound media marker: ${item.type}\n`, | |
| ); | |
| continue; | |
| } | |
| const file = readOutboundMedia(item.path, this.config.cwd); | |
| const upload = await client.uploadMedia(file.data, { | |
| type: item.type, | |
| filename: file.fileName, | |
| }); | |
| const mediaId = extractMediaId(upload); | |
| if (!mediaId) { | |
| throw new Error('WeCom media upload failed: missing media_id'); | |
| } | |
| await client.sendMediaMessage(chatId, item.type, mediaId); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new Critical issues found beyond the existing comments. The following are additional Suggestion-level findings from a second review pass.
— qwen3.7-max via Qwen Code /review
| ] as const; | ||
|
|
||
| const DEDUP_TTL_MS = 5 * 60 * 1000; | ||
| const MAX_OUTBOUND_MEDIA_BYTES = 20 * 1024 * 1024; |
There was a problem hiding this comment.
[Suggestion] MAX_OUTBOUND_MEDIA_BYTES is used at line 238 to gate inbound attachment downloads, but the name says "OUTBOUND". A maintainer adjusting the outbound upload limit would inadvertently change the inbound threshold.
| const MAX_OUTBOUND_MEDIA_BYTES = 20 * 1024 * 1024; | |
| const MAX_MEDIA_BYTES = 20 * 1024 * 1024; |
Then update both usages (lines 238 and 483) to MAX_MEDIA_BYTES.
— qwen3.7-max via Qwen Code /review
| ); | ||
| }); | ||
|
|
||
| await client.connect(); |
There was a problem hiding this comment.
[Suggestion] If client.connect() throws (auth failure, DNS error, TLS timeout), event handlers were already registered on lines 109–126 but the client is never cleaned up. The local client goes out of scope with live event handler closures and potentially an open WebSocket.
Consider wrapping in try/catch:
try {
await client.connect();
} catch (err) {
client.disconnect();
throw err;
}— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| for (const item of media) { | ||
| if (item.type !== 'image') { |
There was a problem hiding this comment.
[Suggestion] When an outbound marker is not image (e.g., [FILE: ...], [VIDEO: ...]), the marker has already been stripped from cleanedText by parseOutboundMediaMarkers, but the media is never uploaded. The user sees the reference silently removed from the reply with only a stderr warning.
Consider either leaving non-image markers in the text or replacing them with an inline placeholder like (file: x.pdf — upload not supported) so the recipient has feedback that something was elided.
— qwen3.7-max via Qwen Code /review
| } | ||
| await this.handleInbound(envelope); | ||
| } catch (err) { | ||
| if (messageId) this.seenMessages.delete(messageId); |
There was a problem hiding this comment.
[Suggestion] When handleInbound throws after downloadAttachments succeeded, the catch block rolls back the dedup entry but does not clean up temp files written to tmpdir()/channel-files/<uuid>/. When WeCom redelivers the message (since dedup was rolled back), new temp files are written to a different UUID directory. Each redelivery accumulates orphan directories.
Consider tracking attachment file paths and cleaning them up in the catch block:
} catch (err) {
if (messageId) this.seenMessages.delete(messageId);
for (const att of attachments ?? []) {
if (att.filePath) try { unlinkSync(att.filePath); } catch {}
}
throw err;
}— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| private async downloadAttachments( |
There was a problem hiding this comment.
[Suggestion] downloadAttachments downloads files sequentially with for (const ref of refs) { await client.downloadFile(...) }. For mixed messages with 2–3 attachments, latency scales linearly. Consider parallelizing:
const downloaded = await Promise.all(
refs.map(async (ref) => ({
ref,
result: await client.downloadFile(ref.url, ref.aesKey),
})),
);— qwen3.7-max via Qwen Code /review
| fileName?: string; | ||
| } | ||
|
|
||
| function collectInboundMediaRefs( |
There was a problem hiding this comment.
[Suggestion] The msg_item loop in collectInboundMediaRefs only extracts image items from mixed messages. If a WeCom user sends a mixed message containing a file or video (e.g., text + file), those attachments are silently skipped — no file is downloaded and no fallback text like (file) or (video) is generated.
Extend the loop to handle file and video item types:
if (itemType === 'image') add('image', getRecord(record, 'image') ?? {});
if (itemType === 'file') add('file', getRecord(record, 'file') ?? {});
if (itemType === 'video') add('video', getRecord(record, 'video') ?? {});— qwen3.7-max via Qwen Code /review
| @@ -128,7 +134,7 @@ export async function parseChannelConfig( | |||
| ); | |||
There was a problem hiding this comment.
[Suggestion] The return statement re-reads ~13 fields (senderPolicy, allowedUsers, cwd, model, etc.) from the original unresolved rawConfig instead of resolvedRawConfig. Since these explicit assignments come after ...resolvedRawConfig, they override any env-var expansion the plugin-required loop applied to fields with matching names.
No current plugin triggers this (WeCom's botId/secret are not among these fields), but a future plugin declaring e.g. model as a requiredConfigFields entry would silently lose its env expansion. Consider reading from resolvedRawConfig consistently:
senderPolicy: (resolvedRawConfig['senderPolicy'] as ...) || 'allowlist',
cwd: resolvePath((resolvedRawConfig['cwd'] as string) || defaultCwd),— qwen3.7-max via Qwen Code /review
| const data = downloaded.buffer; | ||
| if (data.length > MAX_OUTBOUND_MEDIA_BYTES) { | ||
| process.stderr.write( | ||
| `[WeCom:${this.name}] skipping oversized attachment (${data.length} bytes): ${sanitizeLogText( |
There was a problem hiding this comment.
[Critical] This warning includes the WeCom media download URL for oversized attachments. sanitizeLogText() only makes the line safe to print; it does not redact the temporary private URL, so any user who sends an over-limit attachment can force that locator into stderr or log aggregation.
Suggested fix: log only non-sensitive metadata, for example the media type and byte count, and omit ref.url entirely.
— GPT-5 via Qwen Code /review
| return { | ||
| debug: () => {}, | ||
| info: () => {}, | ||
| warn: (...parts) => write('warn', parts), |
There was a problem hiding this comment.
[Critical] The replacement SDK logger still forwards every warn/error argument to stderr. The installed SDK has warn paths such as unknown/invalid frame handling that pass JSON.stringify(frame), which can include message bodies, media URLs, AES keys, and other user data; sanitizeLogText() truncates and neutralizes controls but does not redact those values.
Suggested fix: do not forward SDK variadic args by default. Log only a static/allowlisted warning label, or explicitly redact structured frame data before writing it.
— GPT-5 via Qwen Code /review
| if (explicitBoolean !== undefined) return explicitBoolean; | ||
|
|
||
| const mentions = collectMentionValues(body); | ||
| if (!mentions.length) return undefined; |
There was a problem hiding this comment.
[Critical] Empty mention metadata is treated the same as missing metadata. If WeCom sends mentions: [] or mentioned_list: [] for an unmentioned group message, collectMentionValues() returns an empty array, this returns undefined, and line 210 falls back to true, bypassing requireMention.
Suggested fix: track whether any mention-list key is present separately from the collected values. Return values.some(...) when metadata was present, even if the list is empty; return undefined only when no boolean flag and no mention-list field exist.
— GPT-5 via Qwen Code /review
| }; | ||
|
|
||
| try { | ||
| const attachments = await this.downloadAttachments(body); |
There was a problem hiding this comment.
[Critical] Attachments are downloaded before handleInbound() lets ChannelBase apply the group/sender gates. With the new explicit mention handling, an unmentioned group image/file can still trigger network download and /tmp/channel-files writes before requireMention drops the message.
Suggested fix: run the gate before downloading attachments, or add a lightweight ChannelBase preflight method so rejected group/sender messages return before media fetching while still preserving any intended group-history behavior.
— GPT-5 via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| const resolvedRawConfig = { ...rawConfig }; |
There was a problem hiding this comment.
[Critical] This fix only works when tests call parseChannelConfig() directly. In the real channel startup path, loadSettings() has already run resolveEnvVarsInObject() over settings.merged.channels, so by the time this copy is made, token: '$TEST_TOKEN' has already become the literal secret value. If that value itself starts with $, the later credential parsing calls resolveEnvVars() again and fails with Environment variable TOKEN_LITERAL_VALUE is not set.
Suggested fix: make env expansion happen in one layer only, and cover the production path in a regression test, e.g. settings load/loadChannelsConfig() -> parseConfiguredChannels() with an env value whose literal secret starts with $.
— GPT-5 via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Additional finding (overlaps with existing comment at line 529): safeRealpath(join(tmpdir(), 'channel-files')) returns undefined if the directory has never been created. It is only created lazily by downloadAttachments via mkdirSync. Before the first inbound attachment arrives, allowedDirs is empty and ALL outbound media reads fail with "Media path outside allowed outbound directory" — even for files inside the intended path. Fix: add mkdirSync(join(tmpdir(), 'channel-files'), { recursive: true }) in connect() or before computing allowedDirs.
— qwen3.7-max via Qwen Code /review
| await channel.sendMessage('chat-1', 'a'.repeat(3900)); | ||
|
|
||
| expect(client.sendMessage).toHaveBeenCalledTimes(2); | ||
| const first = client.sendMessage.mock.calls[0]?.[1] as { |
There was a problem hiding this comment.
[Critical] [typecheck] tsc --noEmit reports 4 errors here and at line 384:
- TS2352: Conversion of type
undefinedto type{ markdown: { content: string } }may be a mistake - TS2493: Tuple type
[]of length0has no element at index1
The vi.fn() mock lacks explicit parameter types, so TypeScript infers zero arguments and mock.calls becomes an empty tuple. Add type parameters to the mock:
| const first = client.sendMessage.mock.calls[0]?.[1] as { | |
| const first = client.sendMessage.mock.calls[0]?.[1] as unknown as { | |
| markdown: { content: string }; | |
| }; | |
| const second = client.sendMessage.mock.calls[1]?.[1] as unknown as { | |
| markdown: { content: string }; | |
| }; |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const { cleanedText, media } = parseOutboundMediaMarkers(text); | ||
| for (const chunk of splitMarkdownChunks(cleanedText)) { |
There was a problem hiding this comment.
[Suggestion] The text chunk loop has no per-chunk error handling. If client.sendMessage() rejects (WebSocket drop, SDK timeout), the unhandled rejection aborts all remaining text chunks AND the subsequent media loop with no stderr logging. Both DingTalk (DingtalkAdapter.ts lines 317-339) and Feishu (FeishuAdapter.ts lines 717-751) wrap each chunk send in a try/catch that logs failures and continues.
| for (const chunk of splitMarkdownChunks(cleanedText)) { | |
| for (const chunk of splitMarkdownChunks(cleanedText)) { | |
| try { | |
| await client.sendMessage(chatId, { | |
| msgtype: 'markdown', | |
| markdown: { content: chunk }, | |
| }); | |
| } catch (err) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] failed to send markdown chunk: ${sanitizeLogText(String(err), 200)}\n`, | |
| ); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| function splitMarkdownChunks(text: string): string[] { |
There was a problem hiding this comment.
[Suggestion] splitMarkdownChunks splits by byte budget with no fenced-code-block awareness. When a ``` code block spans a chunk boundary, the first chunk ends with an unclosed fence and the next starts mid-code — both render as broken markdown in WeCom.
Both DingTalk (packages/channels/dingtalk/src/markdown.ts lines 87-156) and Feishu (packages/channels/feishu/src/markdown.ts lines 280-328) track inCode state and close/reopen fences at chunk boundaries:
// DingTalk pattern:
let inCode = false;
const flush = (keepCodeOpen = inCode) => {
if (keepCodeOpen) { buf += '\n```'; }
chunks.push(buf);
buf = keepCodeOpen ? '```' : '';
};Consider extracting into a shared markdown.ts or matching the DingTalk/Feishu pattern.
— qwen3.7-max via Qwen Code /review
| process.stderr.write(`[WeCom:${this.name}] Disconnected.\n`); | ||
| } | ||
|
|
||
| async sendMessage(chatId: string, text: string): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] All text chunks are sent first (lines 145-149), then all media (lines 162-183). When the agent's response interleaves text and media markers (e.g., "Analysis:\n[IMAGE: chart.png]\nSummary:"), the WeCom user receives one combined text message followed by all images, rather than the intended text-image-text sequence.
Process text and media in document order: split the original text at marker boundaries, then iterate segments — send each text segment as markdown, then the associated media immediately after.
— qwen3.7-max via Qwen Code /review
| const MAX_OUTBOUND_MEDIA_BYTES = 20 * 1024 * 1024; | ||
| const MARKDOWN_CHUNK_BYTES = 3800; | ||
|
|
||
| export class WeComChannel extends ChannelBase { |
There was a problem hiding this comment.
[Suggestion] Every other WebSocket-based IM adapter overrides supportsProactiveSend() to return true (DingTalk line 342, Feishu line 167, Telegram line 63). Without this override, the base class default returns false, causing /loop messages to throw "Channel does not support proactive loop messages." — even though the WeCom SDK's sendMessage() works over the persistent WebSocket at any time.
| export class WeComChannel extends ChannelBase { | |
| export class WeComChannel extends ChannelBase { | |
| override supportsProactiveSend(): boolean { | |
| return true; | |
| } |
— qwen3.7-max via Qwen Code /review
| }; | ||
|
|
||
| if (!(await this.preflightInbound(envelope))) return; | ||
|
|
There was a problem hiding this comment.
[Critical] onMessage calls this.preflightInbound(envelope) here, then calls this.handleInbound(envelope) at line 234 — but ChannelBase.handleInbound also calls preflightInbound internally. The group gate and sender gate execute twice per message.
Impact: recordPendingGroupHistory fires twice (corrupting group context replay with duplicated entries) and onPairingRequired fires twice (sending duplicate pairing codes). Any future success-path side effect added to preflightInbound will also double-fire, but only for WeCom messages.
Suggested fix: remove the explicit preflightInbound call here and move attachment download into an override of handleInbound that runs after the parent's preflight. Alternatively, extract the post-preflight portion of handleInbound into a separate protected method (e.g., processInbound) that onMessage calls after its own preflight.
— qwen3.7-max via Qwen Code /review
| } | ||
| client.on('error', (err) => { | ||
| process.stderr.write( | ||
| `[WeCom:${this.name}] SDK error: ${sanitizeLogText(String(err), 200)}\n`, |
There was a problem hiding this comment.
[Critical] Only message.* and error events are registered. No close, disconnect, or reconnect handler exists.
When the WebSocket connection drops (network blip, server restart, idle timeout), the channel silently stops receiving messages. this.client remains set, so sendMessage will attempt to send into a dead connection. No log output indicates the connection is gone — this is the most likely production failure mode for a long-running channel.
Suggested fix: register handlers for the SDK's connection lifecycle events. On close/disconnect, set this.client = undefined and log the transition. Consider adding a reconnect strategy.
client.on('close', () => {
process.stderr.write(`[WeCom:${this.name}] WebSocket closed.\n`);
this.client = undefined;
});— qwen3.7-max via Qwen Code /review
| for (const ref of refs) { | ||
| const downloaded = await client.downloadFile(ref.url, ref.aesKey); | ||
| const data = downloaded.buffer; | ||
| if (data.length > MAX_MEDIA_BYTES) { |
There was a problem hiding this comment.
[Critical] Two inbound media security issues at this download site:
-
SSRF:
ref.urlis extracted from the message body with no domain or scheme validation. A crafted payload could supplyhttp://169.254.169.254/latest/meta-data/orfile:///etc/passwdURLs. The Feishu adapter validates media URLs against expected CDN domains before download. -
OOM before size check:
client.downloadFile()buffers the entire response into a Node.jsBufferbefore theMAX_MEDIA_BYTEScheck runs here. A message referencing a multi-GB resource causes heap exhaustion before the size cap triggers.
Suggested fix:
- Validate
ref.urlagainst WeCom CDN host patterns before passing todownloadFile - Issue a HEAD request to check
Content-Lengthbefore full download, or use streaming with early abort (mirroring the Feishu pattern inpackages/channels/feishu/src/media.ts)
— qwen3.7-max via Qwen Code /review
| client.disconnect(); | ||
| throw err; | ||
| } | ||
| this.client = client; |
There was a problem hiding this comment.
[Critical] Event handlers are registered on the local client at lines 112–127, but this.client = client is only assigned here — after await client.connect() resolves. If the SDK delivers messages during the connect handshake (buffered/delayed messages from the server), onMessage runs but downloadAttachments reads this.client which is still undefined and silently returns []. All attachments are dropped without any log.
Suggested fix: assign this.client = client before await client.connect(). The failure path already calls client.disconnect(), and disconnect() sets this.client = undefined, so cleanup remains correct.
this.client = client; // assign before connect so handlers can use it
try {
await client.connect();
} catch (err) {
this.client = undefined;
client.disconnect();
throw err;
}— qwen3.7-max via Qwen Code /review
|
|
||
| const allowedDirs = [ | ||
| safeRealpath(join(tmpdir(), 'channel-files')), | ||
| safeRealpath('/tmp/channel-files'), |
There was a problem hiding this comment.
[Suggestion] safeRealpath returns undefined for non-existent paths. On a fresh deployment (no prior inbound downloads that create tmpdir()/channel-files via mkdirSync), both entries in allowedDirs are undefined, making the array empty after filter. Every outbound media send then fails with "Media path outside allowed outbound directory" — even for files legitimately placed in the expected location. The error message is misleading because the real cause is that the allowed directory hasn't been created yet.
Suggested fix: create the allowed directories eagerly before resolving their real paths:
const candidates = [join(tmpdir(), 'channel-files'), '/tmp/channel-files'];
for (const dir of candidates) mkdirSync(dir, { recursive: true });
const allowedDirs = candidates.map(safeRealpath).filter((dir): dir is string => Boolean(dir));— qwen3.7-max via Qwen Code /review
| const isGroup = getString(body, 'chattype') === 'group'; | ||
| const chatId = getString(body, 'chatid') || senderId; | ||
| if (!chatId || !senderId) return; | ||
|
|
There was a problem hiding this comment.
[Suggestion] Messages with empty chatId or senderId are silently dropped with no log output. If the WeCom SDK delivers a payload with an unexpected structure (new event type, API version change, malformed frame), messages vanish without trace. Under load, there is no way to detect lost traffic.
Suggested fix:
if (!chatId || !senderId) {
process.stderr.write(
`[WeCom:${this.name}] dropping message ${messageId || '(no id)'}: missing ${!chatId ? 'chatId' : 'senderId'}\n`,
);
return;
}— qwen3.7-max via Qwen Code /review
| name, | ||
| channelsConfig[name] as Record<string, unknown>, | ||
| process.cwd(), | ||
| { resolveEnvVars: false }, |
There was a problem hiding this comment.
[Suggestion] { resolveEnvVars: false } is passed here (and in runtime.ts:208) to prevent double-resolution since loadSettings() already resolves env vars. However, this also disables the channel-level validation that threw clear errors for unset variables (e.g., "Environment variable WECOM_SECRET is not set"). The settings-layer resolver silently preserves unset vars as literal strings.
Impact: users with a typo in their env var name or a missing export will see opaque SDK-level auth errors instead of a clear diagnostic. This regression affects all channels, not just WeCom.
Suggested fix: add a post-resolution check that flags credential fields still looking like unresolved $VAR placeholders:
function assertNoUnresolvedEnvVars(value: string, field: string, channelName: string): void {
if (/^\$[A-Z_][A-Z0-9_]*$/i.test(value)) {
throw new Error(`Channel "${channelName}" field "${field}" appears to reference an unset environment variable: ${value}`);
}
}— qwen3.7-max via Qwen Code /review
| (a === 192 && b === 168) | ||
| ); | ||
| } | ||
| if (ipVersion === 6) { |
There was a problem hiding this comment.
[Critical] IPv4-mapped IPv6 addresses bypass the SSRF blocklist. isIP('::ffff:169.254.169.254') returns 6, entering the IPv6 branch, but none of the IPv6 deny patterns (::1, fc*, fd*, fe80:*) match. This allows SSRF to https://[::ffff:169.254.169.254]/latest/meta-data/ (cloud metadata), ::ffff:127.0.0.1 (loopback), ::ffff:10.x.x.x (private), etc.
| if (ipVersion === 6) { | |
| if (ipVersion === 6) { | |
| const v4Mapped = host.match(/^::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/); | |
| if (v4Mapped) { | |
| const parts = v4Mapped[1]!.split('.').map((p) => Number(p)); | |
| const [a = 0, b = 0] = parts; | |
| return !( | |
| a === 0 || | |
| a === 10 || | |
| a === 127 || | |
| (a === 169 && b === 254) || | |
| (a === 172 && b >= 16 && b <= 31) || | |
| (a === 192 && b === 168) | |
| ); | |
| } | |
| return !( | |
| host === '::1' || | |
| host.startsWith('fc') || | |
| host.startsWith('fd') || | |
| host.startsWith('fe80:') | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| return false; | ||
| } | ||
| try { | ||
| const response = await fetch(ref.url, { |
There was a problem hiding this comment.
[Critical] The fetch() HEAD request uses the default redirect: 'follow'. An attacker can supply a media URL like https://attacker.com/redirect that passes isSafeInboundMediaUrl, but the server responds with 302 Location: http://169.254.169.254/.... The fetch follows the redirect transparently, bypassing the URL safety check entirely and making requests to internal services.
| const response = await fetch(ref.url, { | |
| const response = await fetch(ref.url, { | |
| method: 'HEAD', | |
| redirect: 'manual', | |
| signal: AbortSignal.timeout(10_000), | |
| }); | |
| if (response.status >= 300 && response.status < 400) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] skipping ${ref.type} attachment with redirect.\n`, | |
| ); | |
| return false; | |
| } |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Automated Code Review
13 findings (6 Critical, 7 Suggestions) from 9 parallel review agents + deterministic analysis (tsc + eslint clean). All 439 tests pass.
Critical
sanitizeFileNameuses\w(ASCII-only) — CJK filenames collide, causing silent attachment overwritesArray.from(line.slice(index))[0]is O(n²) in the markdown chunker's character loopcleanupUntrackedAttachmentDirsForSessionscans ALL message dirs on every prompt-end eventdisconnectReconnectFallback30s timer missing.unref()— blocks graceful process exitkickReconnectRetry5-min timer missing.unref()— zombie process after giving up- Preflight-rejected messages added to
seenMessages— blocks WeCom retries for 5 minutes
Suggestions
processInboundmissing JSDoc warning about bypassing security gatesonPairingRequiredrejection not caught — unhandled promise rejectionstart.tsvsruntime.tsuse differentresolveEnvVarsmodes — inconsistent errors- Double-resolution risk for fields in both required and env-resolvable lists
preflightInboundmixed sync/async return type is a truthy-Promise footgun- Monolithic 2022-line file vs sibling adapters' split structure
- Text sent before media upload — partial failure causes duplicate text
| } | ||
| } | ||
|
|
||
| function sanitizeFileName(name: string | undefined): string { |
There was a problem hiding this comment.
[Critical] sanitizeFileName uses \w in [^\w.-], which is ASCII-only ([a-zA-Z0-9_]). All non-ASCII characters — common in WeCom filenames from CJK users — are replaced with _. Multiple attachments with non-ASCII names (e.g., 報告.pdf, 写真.jpg) in the same message all collapse to __.ext, silently overwriting earlier files.
Fix: Use a Unicode-aware character class:
return base.replace(/[^\p{L}\p{N}._-]/gu, '_').replace(/^\.+/, '_');| ? '```' | ||
| : line.startsWith('~~~', index) | ||
| ? '~~~' | ||
| : (Array.from(line.slice(index))[0] ?? ''); |
There was a problem hiding this comment.
[Critical] Array.from(line.slice(index))[0] allocates an array of every remaining code point from index to end-of-line just to extract a single character. This runs inside the character-walking for loop, making the chunker O(n²) on long lines.
For outbound messages with very long lines (base64 data, unwrapped logs), a 10,000-character line creates ~10,000 arrays totaling ~50M code-point allocations.
Fix: Use codePointAt + fromCodePoint:
const cp = line.codePointAt(index);
const token = cp !== undefined ? String.fromCodePoint(cp) : '';| if (!dirs) return; | ||
| const trackedDirs = new Set<string>(); | ||
| for (const messageDirs of this.attachmentDirsByMessage.values()) { | ||
| for (const dir of messageDirs) trackedDirs.add(dir); |
There was a problem hiding this comment.
[Critical] cleanupUntrackedAttachmentDirsForSession iterates ALL values of this.attachmentDirsByMessage to build a trackedDirs Set on every invocation. It's called from onPromptEnd and onPromptBufferDropped, so this full scan fires on every prompt completion.
Under sustained load with many concurrent messages carrying attachments, this becomes O(totalMessages × dirsPerMessage) on the event loop.
Fix: Maintain a reverse index dirsToMessage: Map<string, string> mapping each attachment dir to its messageId, so the 'is this dir tracked?' check becomes a single Map lookup.
| ): void { | ||
| this.clearDisconnectReconnectFallback(); | ||
| const formattedReason = formatDisconnectReason(reason); | ||
| this.disconnectReconnectFallback = setTimeout(() => { |
There was a problem hiding this comment.
[Critical] This 30-second setTimeout is never .unref()'d, unlike other timers in this file (lines 239, 905, 994, 1016, 1033, 1635 all call .unref?.()).
If the WebSocket disconnects and the SDK doesn't reconnect, this timer prevents Node.js graceful process exit. During deploys or SIGTERM, the process hangs for 30 seconds before being force-killed.
Fix: Add this.disconnectReconnectFallback.unref?.(); after the setTimeout assignment.
| delayMs = KICK_RECONNECT_RETRY_MS, | ||
| reconnectReason = 'server kick', | ||
| ): void { | ||
| this.kickReconnectRetry = setTimeout(() => { |
There was a problem hiding this comment.
[Critical] This setTimeout (default 5-minute delay) is never .unref()'d.
After exhausting all retry cycles, the 'manual intervention required' log fires but this timer keeps the process alive. Operators see 'gave up' in logs but the process is still holding resources and will silently retry in 5 minutes. Deeply confusing during incident response.
Fix: Add this.kickReconnectRetry.unref?.(); after the setTimeout assignment at line 925.
| @@ -157,6 +157,7 @@ async function startSingle(name: string, proxy?: string): Promise<void> { | |||
| config = await parseChannelConfig( | |||
There was a problem hiding this comment.
[Suggestion] start.ts (single-channel mode) calls parseChannelConfig with the default resolveEnvVars: true, while runtime.ts (multi-channel) and daemon mode pass resolveEnvVars: 'available'. The two modes produce different error messages for the same misconfigured env var — true mode gives a terse message, while 'available' mode adds the actionable hint 'Set the variable or remove the $ prefix'.
Users running qwen channel start mychannel get less helpful error messages than other entry points for the exact same config mistake.
Fix: Pass { resolveEnvVars: 'available' } in start.ts as well.
| } | ||
| } | ||
| for (const field of plugin.envResolvableConfigFields ?? []) { | ||
| const value = rawConfig[field]; |
There was a problem hiding this comment.
[Suggestion] A config field declared in both plugin.requiredConfigFields and plugin.envResolvableConfigFields would be resolved twice: first in the required loop (line 148), then again in the env-resolvable loop. If the resolved value happens to start with $, the second pass misinterprets it as an env var reference.
No current plugin has this overlap, but future plugin authors could easily double-declare a field. The error would be a confusing 'Environment variable ecret_value is not set' at startup.
Fix: Track resolved fields in a Set<string> and skip already-resolved fields in the envResolvableConfigFields loop.
|
|
||
| async handleInbound(envelope: Envelope): Promise<void> { | ||
| // 1. Group gate: policy + allowlist + mention gating | ||
| protected preflightInbound(envelope: Envelope): boolean | Promise<boolean> { |
There was a problem hiding this comment.
[Suggestion] preflightInbound returns boolean | Promise<boolean>. A future adapter that does if (this.preflightInbound(envelope)) (forgetting await) will always get a truthy Promise object and bypass the gate entirely.
The sync fast path saves one microtask per message — not worth the footgun.
Fix: Make preflightInbound always return Promise<boolean> (always async).
| @@ -0,0 +1,2022 @@ | |||
| import { constants, lstatSync, mkdirSync, realpathSync, rmSync } from 'node:fs'; | |||
There was a problem hiding this comment.
[Suggestion] The adapter is a single monolithic file (~2022 lines, 53 standalone functions) containing channel logic, markdown chunking, IPv4/IPv6 validation, HTTPS download with SSRF protection, media I/O, and WebSocket reconnection state machines. Other adapters split concerns into separate files: feishu/ has markdown.ts and media.ts, dingtalk/ has markdown.ts and media.ts.
The IP-address validation code alone (~200 lines of hand-rolled IPv4/IPv6 parsing) is security-sensitive and deserves its own module with focused tests.
Fix: Extract at minimum media.ts, ip-address.ts, and markdown.ts into separate files, following the pattern of sibling adapters.
| return true; | ||
| } | ||
|
|
||
| async sendMessage(chatId: string, text: string): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] sendMessage delivers markdown text chunks via client.sendMessage before attempting media uploads. If media upload fails partway through, the text has already been delivered but the media is incomplete. The caller treats this as a send failure and may retry, leading to duplicate text delivery.
Users may see the text portion appear multiple times without the intended media.
Fix: Consider uploading all media first, then sending text + media references atomically. Or track delivered chunks and suppress retries that would duplicate them.
|
Qwen Code review timed out. Qwen review timed out after 120 minutes. For large PRs, retry with a longer timeout by commenting: |
…l leakage The guardedHttpsDownload error messages included rawUrl (truncated to 120 chars), which leaks private WeCom media download URLs into stderr and log aggregation systems. Remove the URL from redirect and HTTP error messages.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review via qwen review --comment (9 parallel agents + verification pass).
Test results: 2 failed / 120 passed. The 2 failures are caused by commit 7d634af2f which correctly removed media URLs from error messages but did not update the corresponding test assertions.
4 findings below are unique — not duplicating the 29 existing inline comments from prior review rounds.
| expect(stderr).toHaveBeenCalledWith( | ||
| expect.stringContaining('redirected media URL'), | ||
| ); | ||
| expect(stderr).toHaveBeenCalledWith( |
There was a problem hiding this comment.
[test] This assertion fails. Commit 7d634af2f correctly removed media URLs from the guardedHttpsDownload error messages ("redirected media URL" no longer includes the URL), but this test still asserts that stderr contains the URL string. Either remove this assertion or update the error message to include a redacted/non-sensitive URL reference.
| expect(stderr).toHaveBeenCalledWith( | ||
| expect.stringContaining('media download failed: HTTP 500'), | ||
| ); | ||
| expect(stderr).toHaveBeenCalledWith( |
There was a problem hiding this comment.
[test] Same issue as line 2240 — the error message "media download failed: HTTP 500" no longer includes the URL, so this assertion fails. The fix: remove the URL assertion or include a non-sensitive identifier in the error message.
| } | ||
| this.seenMessages.clear(); | ||
| this.inFlightMessages.clear(); | ||
| this.cleanupAllAttachmentDirs(); |
There was a problem hiding this comment.
[correctness] cleanupAllAttachmentDirs() uses rmSync synchronously while in-flight onMessage calls may still be writing attachment files via async downloadAttachments. The disconnectGeneration guard in onMessage (line 421) is checked between downloads but not between individual file writes inside a single download.
Consider either:
- Making
disconnect()async and awaiting in-flight writes before cleanup, or - Adding a
disconnectGenerationcheck insidedownloadAttachmentsbefore eachwriteFilecall (the existing check at line 472 is between iterations, not before the write at line 488+).
| } | ||
|
|
||
| function findCodeRanges(text: string): Array<[number, number]> { | ||
| const ranges: Array<[number, number]> = []; |
There was a problem hiding this comment.
[correctness] findCodeRanges only detects fenced code blocks (``` / ~~~) and inline backticks. It does not handle Markdown indented code blocks (4+ spaces or tab indent). If a user types [IMAGE: path] inside an indented code block, parseOutboundMediaMarkers would incorrectly strip it as a real media marker.
Indented code blocks are uncommon in chat contexts, but this is a defense-in-depth gap worth noting.
| .join(' '); | ||
| } | ||
| try { | ||
| return JSON.stringify(record); |
There was a problem hiding this comment.
[security] For unknown error record shapes, formatSdkError falls back to JSON.stringify(record). If the error object contains secret, botId, or other sensitive config fields, they would be serialized to stderr. The sanitizeLogText truncation at call sites provides some protection but does not redact field names/values.
Consider picking only known-safe fields from the record, or masking known credential field names before serialization.
- Remove stale URL assertions from media download error tests (the error messages no longer include raw URLs after the credential-leak fix) - Redact sensitive fields (secret, aeskey, token, password, authorization) in formatSdkError's JSON.stringify fallback to prevent credential leakage in logs - Add indented code block detection to findCodeRanges so [IMAGE: path] inside 4-space/tab-indented code is not stripped as a media marker - Add disconnectGeneration guard before mkdirSync in downloadAttachments to prevent orphaned temp directories when disconnect() races with in-flight attachment downloads
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Qwen Code /review — PR #6224
Verdict: Request changes — 2 critical issues and 4 suggestions not covered by existing review comments.
This PR adds a WeCom (WeChat Work) intelligent robot channel adapter with ~6800 new lines across 23 files. The SSRF defense-in-depth, path traversal guards, and test coverage (465 tests, all passing) are well-executed. The ChannelBase refactoring (preflightInbound/processInbound split) is clean. However, reconnection lifecycle and error propagation have several correctness issues that could cause silent process exits or duplicated messages in production.
Deterministic analysis: tsc + eslint clean (0 findings). All 465 tests pass.
Review stats: 21 findings from 9 parallel agents, 2 rejected during verification, 19 verified. 13 overlap with existing comments on this PR. 6 new findings posted below.
New findings (not in existing comments)
Critical
WeComAdapter.ts:947— Reconnection timers use.unref(), causing silent process exitWeComAdapter.ts:884— "Manual intervention required" but code auto-retries; cycle counter never resets
Suggestions
WeComAdapter.ts:1324— Code fence regex matches inline triple-backticksChannelBase.ts:2846— Stale comment after handleInbound → processInbound refactorWeComAdapter.ts:1659— Idle-only timeout enables trickle DoS on media downloadsWeComAdapter.ts:242—client.disconnect()in catch block not wrapped in try/catch
Existing comments cover well
The 30 existing review comments on this PR already cover: mention handling, inbound size limits, SDK logger leakage, uploadMedia options mismatch, markdown normalization, length splitting, env-var double-expansion, and test coverage gaps.
— qwen3.7-max via Qwen Code /review
| this.kickReconnectAttempts = 0; | ||
| this.startKickReconnect(reason, reconnectReason); | ||
| }, delayMs); | ||
| this.kickReconnectRetry.unref?.(); |
There was a problem hiding this comment.
[Critical] kickReconnectRetry, disconnectReconnectFallback, and the dedup cleanup setInterval (line 826) all call .unref?.() on their timer handles. If the WeCom WebSocket is the sole workload (common for a dedicated channel process), and the connection drops, Node.js will exit silently because no referenced timers or handles remain. The last log line will be "waiting for SDK reconnect" — no crash, no health check failure, no alert.
Suggested fix: Remove .unref?.() from kickReconnectRetry (line 947) and disconnectReconnectFallback (line 927) so the process stays alive during reconnection. Keep dedupTimer unref'd as it is pure housekeeping unrelated to connectivity.
— qwen3.7-max via Qwen Code /review
| this.kickReconnectRetryCycles += 1; | ||
| if (this.kickReconnectRetryCycles >= KICK_RECONNECT_MAX_RETRY_CYCLES) { | ||
| process.stderr.write( | ||
| `[WeCom:${this.name}] reconnect after ${reconnectReason} exhausted ${this.kickReconnectRetryCycles} retry cycles; manual intervention required; retrying later.\n`, |
There was a problem hiding this comment.
[Critical] When kickReconnectRetryCycles >= MAX_RETRY_CYCLES, this logs "manual intervention required; retrying later" but then immediately schedules another 15-minute auto-retry via kickReconnectRetry(). Worse, kickReconnectRetryCycles is never reset in the timer callback (only on success at line 866), so every subsequent retry cycle logs "manual intervention required" again even though auto-recovery is still trying.
Suggested fix: Either actually stop retrying and require manual restart (matching the message), or change the log to "exhausted N retry cycles; will keep retrying every 15m" and reset kickReconnectRetryCycles = 0 in the timer callback.
— qwen3.7-max via Qwen Code /review
| const ranges: Array<[number, number]> = []; | ||
| let fenceStart: number | undefined; | ||
| let fenceToken: FenceToken | undefined; | ||
| for (const match of text.matchAll(/```|~~~/g)) { |
There was a problem hiding this comment.
[Suggestion] Both findCodeRanges (here) and toggleCodeFenceState (line 1442) use /```|~~~/g which matches triple-backticks anywhere in text. CommonMark requires code fences at line starts (up to 3 spaces indent). Inline triple-backtick mentions (e.g., "use ``` for code") would be misclassified as fence tokens, causing parseOutboundMediaMarkers to skip legitimate media markers inside those regions.
| for (const match of text.matchAll(/```|~~~/g)) { | |
| for (const match of text.matchAll(/^(?: {0,3})(```|~~~)/gm)) { |
— qwen3.7-max via Qwen Code /review
| sessionId, | ||
| buffer, | ||
| ); | ||
| // Re-enter handleInbound with the coalesced message |
There was a problem hiding this comment.
[Suggestion] Stale comment — after the handleInbound → processInbound refactor, this code calls this.processInbound(), not this.handleInbound(). These are different methods: handleInbound runs preflight gates while processInbound skips them.
| // Re-enter handleInbound with the coalesced message | |
| // Re-enter processInbound with the coalesced message (preflight already passed) |
— qwen3.7-max via Qwen Code /review
| res.on('error', (err: Error) => finish(err)); | ||
| }, | ||
| ); | ||
| req.setTimeout(10_000, () => { |
There was a problem hiding this comment.
[Suggestion] req.setTimeout(10_000) is a socket idle timeout — it resets on each data chunk. An attacker controlling a media server can send 1 byte every 9 seconds, keeping the connection open indefinitely within the 60-second absolute timeout. No throughput floor is enforced, enabling a trickle/slow-loris DoS.
Suggested fix: Track total bytes received and enforce a minimum throughput (e.g., 64 KB/s). If total / elapsedSeconds drops below the threshold, destroy the request.
— qwen3.7-max via Qwen Code /review
| if (this.connectingClient === client) this.connectingClient = undefined; | ||
| if (this.authentication === authentication) | ||
| this.authentication = undefined; | ||
| client.disconnect(); |
There was a problem hiding this comment.
[Suggestion] client.disconnect() here (in the openClient catch block) is not wrapped in try/catch. If disconnect() itself throws (e.g., accessing a destroyed WebSocket), it replaces the original connection error and masks the root cause.
| client.disconnect(); | |
| try { client.disconnect(); } catch { /* already handled */ } |
— qwen3.7-max via Qwen Code /review
…ction error In the connect() catch block, client.disconnect() could throw (e.g. if the WebSocket was already destroyed), masking the original connection error. Wrap in try/catch so cleanup failures never shadow the root cause.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings — all prior Critical issues (fence-token-switching, disconnect generation guard) are correctly addressed in the 7 fix commits. Downgraded from Approve to Comment: CI Test (ubuntu-latest, Node 22.x) is still in progress.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
| import { tmpdir } from 'node:os'; | ||
| import { Buffer } from 'node:buffer'; | ||
| import { isIP, type LookupFunction } from 'node:net'; | ||
| import { lookup } from 'node:dns/promises'; |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Qwen Code Review — PR #6224
Deterministic pre-check: tsc 0 errors | eslint 0 errors | tests 440/440 pass
Found 2 Critical, 4 Important issues. Inline comments on affected lines below.
Summary
| Severity | Count |
|---|---|
| Critical | 2 |
| Important | 4 |
| Suggestion | 7 |
Critical: kick reconnect counter not reset on fresh kick (#1); sendMessage throws after text already delivered (#2).
Important: misleading "manual intervention required" log while still retrying (#3); code fence tracker assumes exactly 3 backticks (#4); onPromptEnd destroys attachment dirs before buffer drain (#5); coalescedAttachmentMessages Map leaks (#6).
| if (previousConnecting) { | ||
| await previousConnecting.catch(() => {}); | ||
| } | ||
| while (this.kickReconnectAttempts < KICK_RECONNECT_MAX_ATTEMPTS) { |
There was a problem hiding this comment.
[Critical — Correctness] kickReconnectAttempts is not reset when a fresh kicked event arrives.
When reconnectingAfterKick is false and kickReconnectRetry is set, this method clears the retry timer (lines 841–844) but never resets kickReconnectAttempts before the while-loop below.
Scenario: A previous reconnect cycle exhausts 3 attempts (kickReconnectAttempts = 3). scheduleKickReconnectRetry schedules a retry in 5 minutes. Before that timer fires, a fresh kicked event arrives → reconnectAfterKick clears the timer but enters the while-loop with kickReconnectAttempts = 3, so 3 < 3 is false and no reconnect attempt is made.
Suggested fix — add reset after clearing the timer:
if (this.kickReconnectRetry) {
clearTimeout(this.kickReconnectRetry);
this.kickReconnectRetry = undefined;
}
this.kickReconnectAttempts = 0; // ← reset for fresh kick| const message = `[WeCom:${this.name}] ${mediaErrors.length} media send(s) failed (markdown text may already be delivered): ${mediaErrors.join('; ')}`; | ||
| process.stderr.write(`${message}\n`); | ||
| throw new Error(message); | ||
| } |
There was a problem hiding this comment.
[Critical — Partial Delivery] throw after text chunks are already delivered to the recipient.
Text chunks are sent at lines 306–311 via client.sendMessage. If any media upload fails, errors accumulate and this throw signals total failure — but text was already delivered. The caller cannot distinguish "nothing sent" from "text sent, media failed."
Options:
- Return a partial-success result instead of throwing (preferred)
- Remove the
throwand rely on the stderr log for observability - Move media processing before text delivery so failures happen atomically
| this.scheduleKickReconnectRetry( | ||
| reason, | ||
| disconnectGeneration, | ||
| KICK_RECONNECT_LONG_RETRY_MS, |
There was a problem hiding this comment.
[Important — Misleading Log] "manual intervention required" but scheduleKickReconnectRetry is called immediately after — the bot never actually stops retrying.
This is confusing for operators watching logs. Either:
- Actually stop retrying (remove the
scheduleKickReconnectRetrycall), or - Change the message to "manual intervention recommended; will continue retrying" and add a circuit-breaker after N more cycles
| for (const match of line.matchAll(/```|~~~/g)) { | ||
| const token = match[0] as FenceToken; | ||
| if (nextCodeFence === token) { | ||
| nextCodeFence = undefined; |
There was a problem hiding this comment.
[Important — Code Fence Tracking] Regex assumes exactly 3 backticks/tildes.
/\``|~~~/g` only matches 3-character sequences. Markdown allows variable-length fences (4+ backticks/tildes per CommonMark spec). A 4-backtick fence ````` would match once (first 3), incorrectly toggling the state.
The inner loop in splitMarkdownChunks (lines 1407–1413) has the same issue with line.startsWith('\``', index)`.
Impact: A 4-backtick code fence causes subsequent chunks to misclassify code vs prose, potentially splitting code blocks across messages.
Suggested fix:
for (const match of line.matchAll(/`{3,}|~{3,}/g)) {
// track fence by token length for proper open/close matching
}| const lost = buffer.length; | ||
| const coalesced = buffer.map((b) => b.text).join('\n\n'); | ||
| const lastEnvelope = buffer[buffer.length - 1]!.envelope; | ||
| this.notifyPromptBufferDrained( |
There was a problem hiding this comment.
[Important — Ordering] onPromptEnd(chatId, sessionId) at line 704 is called before this buffer drain, without a messageId.
In the WeCom adapter, onPromptEnd without messageId triggers cleanupAttachmentDirsForSession which calls rmSync on temp dirs. By the time this drain fires, attachment directories are already deleted.
The practical impact is partially mitigated since the synthetic envelope below strips attachments/imageBase64/imageMimeType — but this means buffered messages permanently lose their attachments regardless of whether dirs exist.
Consider moving onPromptEnd after the drain, or passing the messageId to scope the cleanup.
| } | ||
| const coalescedMessageIds = this.coalescedAttachmentMessages.get(messageId); | ||
| if (coalescedMessageIds) { | ||
| this.coalescedAttachmentMessages.delete(messageId); |
There was a problem hiding this comment.
[Important — Memory Leak] coalescedAttachmentMessages Map is never cleaned in the loop path.
Cleanup only happens in:
onPromptEndwith matching messageId (line 599)cleanupAllAttachmentDirson disconnect (line 743)
In the loop path, onPromptEnd is called without messageId (ChannelBase.ts:704) and doesn't touch this Map. Each onPromptBufferDrained call adds an entry (line 575) that persists for the connection lifetime.
Entries are small (string arrays), so the leak is bounded, but it grows monotonically with each prompt cycle.
| } | ||
| } | ||
| if (mediaErrors.length > 0) { | ||
| const message = `[WeCom:${this.name}] ${mediaErrors.length} media send(s) failed (markdown text may already be delivered): ${mediaErrors.join('; ')}`; |
There was a problem hiding this comment.
[Critical] After text chunks have been successfully delivered, throwing on media failure causes the caller to retry the entire response — duplicating the already-delivered text in the chat.
The error message itself acknowledges "markdown text may already be delivered." DingTalk and Feishu log failures and continue; they don't throw after partial delivery.
| const message = `[WeCom:${this.name}] ${mediaErrors.length} media send(s) failed (markdown text may already be delivered): ${mediaErrors.join('; ')}`; | |
| if (mediaErrors.length > 0) { | |
| const message = `[WeCom:${this.name}] ${mediaErrors.length} media send(s) failed (markdown text may already be delivered): ${mediaErrors.join('; ')}`; | |
| process.stderr.write(`${message}\n`); | |
| } |
— qwen3.7-max via Qwen Code /review
| void this.reconnectAfterKick(reason, reconnectReason).catch( | ||
| (err: unknown) => { | ||
| process.stderr.write( | ||
| `[WeCom:${this.name}] kick-reconnect failed: ${sanitizeLogText( |
There was a problem hiding this comment.
[Critical] If reconnectAfterKick throws an unexpected error (e.g., from disconnectClientOnly state corruption or from awaiting previousConnecting), this catch handler logs the error but never schedules a future retry. The channel enters a permanent zombie state: disconnected, no client, no pending timers, no recovery path. The only way out is a full process restart.
Consider scheduling a long-delay retry instead of giving up:
process.stderr.write(...);
if (this.kickReconnectRetry === undefined) {
this.scheduleKickReconnectRetry(
reason, this.disconnectGeneration,
KICK_RECONNECT_LONG_RETRY_MS, reconnectReason,
);
}— qwen3.7-max via Qwen Code /review
| this.cleanupAttachmentDirsForMessage(messageId); | ||
| } | ||
| if (!messageId) { | ||
| this.cleanupAttachmentDirsWithoutMessage(attachmentRouteKey); |
There was a problem hiding this comment.
[Critical] When messageId is undefined, this cleans up ALL untracked attachment dirs for the routeKey. If a second concurrent message from the same sender is downloading attachments into the same routeKey's dirs, those dirs get wiped mid-download — the sibling message's files are silently destroyed.
Consider generating a synthetic dedup key for messages without msgid (so the if (messageId) branch at line 364 always has a key), or defer route-based cleanup to a timer rather than running it synchronously in the finally block.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
WeComAdapter.ts:364 |
Messages without msgid bypass all dedup — seenMessages and inFlightMessages guards are all if (messageId)-gated |
Generate a content-hash fallback key (sha256(senderId + chatId + text)) and guard dedup operations on it unconditionally |
WeComAdapter.ts:1899 |
isPublicIpv4 only blocks c === 0 || c === 2 in 192.0.0.0/24, leaving 254 other IANA-reserved addresses classified as "public" |
Replace (a === 192 && b === 0 && (c === 0 || c === 2)) with (a === 192 && b === 0 && c < 256) |
WeComAdapter.ts:1927 |
detectImageMime only tested for PNG magic bytes — GIF, WebP, JPEG, and fallback branches untested |
Add tests for JPEG (0xff 0xd8 0xff), GIF (0x47 0x49 0x46), WebP, and unknown bytes |
WeComAdapter.ts (SSRF path) |
Duplicate DNS resolution per media download — isSafeInboundMediaUrl resolves DNS, then safePublicLookup resolves again |
Cache resolved public IPs from the safety check and pass them to guardedHttpsDownload, or consolidate into a single DNS gate |
WeComAdapter.ts (whole file) |
2058-line monolithic file — DingTalk (1039 lines) and Feishu (2210 lines) both extract markdown.ts and media.ts |
Extract SSRF protection, media handling, and markdown splitting into separate files |
ChannelBase.ts:968-983 |
Three new lifecycle hooks (onPromptBuffered/onPromptBufferDrained/onPromptBufferDropped) lack JSDoc explaining the resource-management contract |
Add JSDoc: "If you override onPromptBuffered to track per-message resources, you MUST also override onPromptBufferDropped to release them" |
config-utils.ts:41 |
resolveConfigEnvVar doesn't handle ${VAR} brace syntax — ${MY_VAR} extracts envName as {MY_VAR} |
Strip braces: if (envName.startsWith('{') && envName.endsWith('}')) envName = envName.slice(1, -1) |
WeComAdapter.ts:1487 |
ensureDirectoryRealpath creates channel-files dir with default permissions (~0o755); per-message dirs use 0o700 |
Add mode: 0o700 to the mkdirSync call |
— qwen3.7-max via Qwen Code /review
| this.disconnectGeneration === disconnectGeneration; | ||
| this.pendingKickReconnect = false; | ||
| if (shouldRetryPendingKick) { | ||
| this.kickReconnectAttempts = 0; |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #6224 feat(channels): add WeCom intelligent robot channel
Build & Tests: ✅ All pass (WeComAdapter: 125, ChannelBase: 287, CLI channel: 56)
Deterministic analysis (tsc + eslint): ✅ Clean
Findings: 14 total (2 Critical, 8 Suggestion, 4 Nice to have)
The two most critical issues:
- Connection flapping in kick-reconnect (WeComAdapter.ts:905) — pending kick unconditionally tears down a freshly-reconnected healthy client
- Test coverage gaps —
onPromptBufferDrainedhook has zero tests;resolveEnvVars: falsemode is untested
Additional test gaps not covered by inline comments:
- Error paths for all three buffer lifecycle hooks (try/catch → stderr) are untested
dropCollectBuffervia the/clearcommand path doesn't testonPromptBufferDroppedenvResolvableConfigFieldsgraceful skip when field is absent from rawConfig is untested
(Note: a stray test comment was accidentally posted at WeComAdapter.ts:905 earlier — please disregard.)
Thanks for this substantial and well-structured PR! The SSRF protection, IPv6 handling, and attachment lifecycle management are thorough.
| this.disconnectGeneration === disconnectGeneration; | ||
| this.pendingKickReconnect = false; | ||
| if (shouldRetryPendingKick) { | ||
| this.kickReconnectAttempts = 0; |
There was a problem hiding this comment.
[Critical] When reconnectAfterKick succeeds and this.client is set, this finally block unconditionally processes any pendingKickReconnect — tearing down the just-established healthy connection and starting a new reconnect cycle.
This causes connection flapping: if two kick events arrive close together, the second triggers a disconnect of the freshly-reconnected client. If the second reconnect then fails, the channel enters a 15-minute retry wait despite having had a working connection moments earlier.
// Suggested fix: guard with a liveness check
if (shouldRetryPendingKick && !this.client) {
this.kickReconnectAttempts = 0;
this.startKickReconnect(reason, reconnectReason);
}| .filter((part): part is string => Boolean(part)) | ||
| .join(' '); | ||
| } | ||
| try { |
There was a problem hiding this comment.
[Suggestion] formatSdkError performs only shallow (top-level) credential redaction. The Object.entries(record).map(...) only checks first-level keys. If the WeCom SDK (which uses axios internally, per the comment at line 1594) emits a nested error like { config: { headers: { authorization: "..." }, data: { secret: "..." } } }, the JSON.stringify(redacted) fallback serializes nested secrets verbatim to stderr.
// Suggested fix: deep redaction via JSON.stringify replacer
return JSON.stringify(record, (key, value) =>
SENSITIVE_ERROR_FIELDS.has(key) ? '[REDACTED]' : value
);The replacer callback is invoked for every nested key, providing deep redaction with zero additional complexity.
| return { safe: false, reason: 'invalid URL' }; | ||
| } | ||
| if (url.protocol !== 'https:') { | ||
| return { safe: false, reason: 'non-HTTPS protocol' }; |
There was a problem hiding this comment.
[Suggestion] isSafeInboundMediaUrl does not check for embedded userinfo credentials. A URL like https://secret@host.example.com/media passes all checks — the URL constructor parses credentials into url.username/url.password, but only url.hostname is examined. Node.js https.request will forward the userinfo as an Authorization: Basic header to the resolved host.
// Suggested fix: add after the protocol check
if (url.username || url.password) {
return { safe: false, reason: 'URL contains embedded credentials' };
}| : `(file: ${attachments[0]?.fileName ?? 'file'})`; | ||
| } | ||
| processStarted = true; | ||
| await this.processInbound(envelope); |
There was a problem hiding this comment.
[Suggestion] WeComChannel is the only adapter that calls this.processInbound() directly (after manually calling this.preflightInbound()). Every other adapter calls this.handleInbound() which encapsulates both steps. The reason is understandable — attachment download must happen between preflight and processing — but if handleInbound ever gains additional logic (rate limiting, metrics, audit hooks), WeComChannel will silently skip it.
Consider adding a comment at this call site:
// Cannot use handleInbound(): attachment download must occur between
// preflight and processing to avoid downloading for rejected messages.
await this.processInbound(envelope);And a reciprocal note in ChannelBase.handleInbound warning that adapters may re-implement its pipeline.
| string[] | ||
| >(); | ||
| private readonly bufferedAttachmentMessages = new Set<string>(); | ||
| private readonly coalescedAttachmentMessages = new Map<string, string[]>(); |
There was a problem hiding this comment.
[Suggestion] coalescedAttachmentMessages has no TTL, no periodic sweep, and no size cap. Cleanup relies exclusively on onPromptEnd receiving the expected messageId. But onPromptEnd is called without messageId on the loop-dispatch path (ChannelBase.ts:704) and the /clear eviction path (ChannelBase.ts:1144), silently orphaning entries.
Unlike seenMessages (which has cleanupSeenMessages on a timer), this map has zero safety-net reclamation. Over days/weeks on a busy channel, orphaned entries accumulate without bound.
Consider including coalescedAttachmentMessages in the periodic cleanup sweep, or storing a timestamp and evicting stale entries.
| disconnectGeneration: number, | ||
| ): void { | ||
| this.clearDisconnectReconnectFallback(); | ||
| const formattedReason = formatDisconnectReason(reason); |
There was a problem hiding this comment.
[Suggestion] The disconnect-reconnect fallback timer is only cleared by incoming messages (clearDisconnectReconnectFallback in messageHandler). There is no listener for SDK reconnection signals (e.g., a subsequent authenticated event) after initial authentication.
In low-traffic channels, if the SDK reconnects internally after a brief WebSocket drop but no inbound message arrives within the 30-second fallback window, the timer fires and tears down the now-working client — causing an unnecessary reconnect storm.
Consider registering a one-shot authenticated listener (scoped to the same connectionGeneration) that calls this.clearDisconnectReconnectFallback().
| reconnectReason, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The log message "manual intervention required; retrying later" is misleading — the adapter continues self-healing via scheduleKickReconnectRetry every 15 minutes. This creates alarming-but-not-actionable log lines repeated indefinitely. Additionally, kickReconnectRetryCycles is never reset after exhaustion (only a successful connect resets it), so every subsequent failed cycle re-emits the same alarming message.
Consider: "exhausted ${N} retry cycles; next attempt in ${delayMs / 60_000} minutes" — and emit the "manual intervention" variant only on the first exhaustion.
|
|
||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] instanceof Promise is fragile for the preflightInbound return type (boolean | Promise<boolean>). If a subclass returns a non-native thenable (e.g., from a library or custom async helper), instanceof Promise is false, the thenable is treated as a truthy boolean, and processInbound runs without waiting for the gate check — bypassing group gating, sender allowlisting, and pairing checks entirely.
// Suggested fix: always await — await on a non-Promise is a no-op
async handleInbound(envelope: Envelope): Promise<void> {
if (!(await this.preflightInbound(envelope))) return;
await this.processInbound(envelope);
}This is already what WeComAdapter does in its direct call path. The micro-cost of await on a synchronous boolean is one microtask hop, negligible for inbound message handling.
|
|
||
| return { | ||
| ...rawConfig, | ||
| ...resolvedRawConfig, |
There was a problem hiding this comment.
[Suggestion] While the spread now correctly uses resolvedRawConfig (fixing plugin field resolution), the explicitly-read generic fields below (instructions, model, approvalMode, cwd, senderPolicy, sessionScope, groupPolicy, groups) still read from the unresolved rawConfig. If a future plugin declares one of these generic field names in envResolvableConfigFields, the resolved value computed above is silently overridden by the unresolved $ENV_VAR literal from rawConfig.
// Suggested fix: read generic fields from resolvedRawConfig
senderPolicy: (resolvedRawConfig['senderPolicy'] as SenderPolicy | undefined) || 'allowlist',Or throw explicitly when a $-prefixed string appears in a non-env-resolvable field, so misconfigurations fail loudly at startup.
| } | ||
| } | ||
|
|
||
| private notifyPromptBufferDrained( |
There was a problem hiding this comment.
[Suggestion — test gap] onPromptBufferDrained is called here before processInbound(syntheticEnvelope). If processInbound rejects (the .catch() handler only logs), the onPromptBufferDrained callback in WeComChannel has already recorded coalesced message IDs in coalescedAttachmentMessages. Those entries will never be cleaned up since the prompt lifecycle never completes.
This hook has zero test coverage — TestChannel overrides onPromptBuffered and onPromptBufferDropped but not onPromptBufferDrained. Consider:
- Calling
onPromptBufferDroppedin the.catch()handler to roll back the drained state - Adding tests that override
onPromptBufferDrainedin TestChannel and verify it fires with correct messageIds
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
|
No new Suggestion-level findings this round — all prior suggestions have been addressed or superseded. — qwen3.7-max via Qwen Code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
9 parallel review agents analyzed this PR across correctness, security, code quality, performance, test coverage, attacker mindset, oncall readiness, future maintainability, and build/test verification.
445 tests pass (127 WeCom + 287 ChannelBase + 31 config-utils). tsc --noEmit and eslint: 0 findings.
The security hardening is excellent — SSRF protection, DNS rebinding guards, and path traversal prevention are all well-implemented and verified. The main concerns are operational (silent failure modes that would be nightmares at 3 AM) and architectural (the preflightInbound/processInbound split creates a security bypass risk for future adapters).
| Severity | Count |
|---|---|
| Critical | 3 |
| Suggestion | 8 |
| Nice to have | 3 |
| process.stderr.write( | ||
| `[WeCom:${this.name}] message handling failed: ${sanitizeLogText( | ||
| formatSdkError(err), | ||
| 200, |
There was a problem hiding this comment.
[Critical — oncall/3AM] The messageHandler catch logs "message handling failed: <error>" without including the messageId. Under load, this makes incident investigation impossible — you cannot correlate errors to specific messages, and cannot tell if one message is repeatedly failing or many different ones.
this.onMessage(payload, connectionGeneration).catch((err: unknown) => {
process.stderr.write(
`[WeCom:${this.name}] message handling failed: ${sanitizeLogText(
formatSdkError(err),
200,
)}\n`,
);
});Suggested: pass the messageId (or logMessageId) into the error context. One approach is to have onMessage return a result object that includes the messageId, or wrap the throw with messageId context before it reaches this catch.
|
|
||
| const client = new ClientCtor(options); | ||
| let authenticated = false; | ||
| const connectionGeneration = this.disconnectGeneration; |
There was a problem hiding this comment.
[Critical — oncall/3AM] No heartbeat/watchdog for silent WebSocket death. The adapter relies entirely on SDK disconnected/kicked events to trigger recovery. If the underlying WebSocket dies silently (TCP half-open connection, network partition, SDK bug that swallows the close frame), no event fires and the adapter sits indefinitely with authenticated = true and zero log output. Messages are silently lost.
This is the #1 3 AM nightmare: the channel appears healthy (process running, no error logs, "Connected" was logged hours ago), but zero messages are being delivered. Discovery only happens via end-user complaints.
Suggested fix — add a watchdog timer:
private lastActivityAt = Date.now();
// In messageHandler: this.lastActivityAt = Date.now();
// Watchdog: if (Date.now() - this.lastActivityAt > STALE_THRESHOLD_MS) {
// log warning + startKickReconnect
// }| * already preflighted, such as during collect-buffer drain. | ||
| */ | ||
| protected async processInbound(envelope: Envelope): Promise<void> { | ||
| // 3. Slash command handling — before session/agent routing |
There was a problem hiding this comment.
[Critical — future maintainer] processInbound is protected and bypasses all security gates (group gating, sender allowlisting, pairing). The JSDoc says "Callers must run preflightInbound() first" but there is no runtime enforcement.
The WeCom adapter calls preflightInbound + processInbound separately in onMessage, establishing a pattern that future adapter authors will copy. If they forget the preflightInbound call in one code path (e.g., a message-edit event handler, a retry path), unauthenticated messages reach the agent silently.
Suggested approaches (any of these):
- Return a
PreflightTokenfrompreflightInboundthatprocessInboundrequires as a parameter - Set a
private preflightPassed = new WeakSet<Envelope>()inpreflightInbound, check it inprocessInbound - At minimum, rename to
unsafeProcessInboundto make the danger visible - Add
@internalJSDoc to signal these are not for general adapter use
| let nextCodeFence = codeFence; | ||
| for (const match of line.matchAll(/```|~~~/g)) { | ||
| const token = match[0] as FenceToken; | ||
| if (nextCodeFence === token) { |
There was a problem hiding this comment.
[Suggestion — correctness] toggleCodeFenceState matches ``` and ~~~ anywhere on a line via line.matchAll(/```|~~~/g). CommonMark requires code fence tokens to be at the beginning of a line (with up to 3 spaces of indentation). A line like See \`` for code ``` details` would toggle fence state incorrectly, corrupting the markdown chunker.
Same issue in findCodeRanges which uses the same mid-line regex pattern.
Suggested fix: anchor fence detection to line beginnings:
/(?:^|\n) {0,3}(```|~~~)/gThis matches CommonMark spec behavior and prevents mid-line backtick sequences from being misinterpreted as fence markers.
| continue; | ||
| } | ||
|
|
||
| let needsLineBreak = Boolean(current); |
There was a problem hiding this comment.
[Suggestion — performance] The splitMarkdownChunks inner loop has O(N × CHUNK_SIZE) complexity for overlong lines. For each character/token, it:
- Builds a candidate string via template literal
${current}${addition} - Calls
Buffer.byteLength(candidate, 'utf8')on a ~3800-byte string - Creates 2-3 new string allocations per iteration
For a 100KB line (base64, minified code, very long URL): ~100K iterations × ~3800 bytes of Buffer.byteLength work each, blocking the event loop for hundreds of milliseconds.
Suggested fix: track byte length incrementally:
let currentByteLength = 0;
// When appending: currentByteLength += Buffer.byteLength(token, 'utf8');
// On flush: currentByteLength = 0;
// Check: currentByteLength + Buffer.byteLength(token, 'utf8') <= MARKDOWN_CHUNK_BYTESThis reduces from O(N × CHUNK_SIZE) to O(N).
| private readonly seenMessages = new Map<string, number>(); | ||
| private readonly inFlightMessages = new Set<string>(); | ||
| private readonly attachmentDirsByMessage = new Map<string, string[]>(); | ||
| private readonly attachmentMessageByDir = new Map<string, string>(); |
There was a problem hiding this comment.
[Suggestion — code quality] The entire adapter lives in a single 2060-line file. Every other adapter of comparable size splits helpers into separate modules:
- Feishu (2210 lines) →
markdown.ts+media.ts - DingTalk (1039 lines) →
markdown.ts+media.ts - QQBot (1551 lines) →
api.ts,accounts.ts,login.ts,send.ts,types.ts
WeCom has at least four extractable helper domains:
- markdown chunking (~90 lines,
splitMarkdownChunks+ helpers) - IP/URL safety (~200 lines,
isPublicIpAddress+ all IPv4/IPv6 helpers) - media I/O (~120 lines,
guardedHttpsDownload+readOutboundMedia) - payload extraction (~200 lines, mention detection + content parsing)
Extracting at minimum markdown.ts, media.ts, and ip-safety.ts would follow the established convention and make each piece independently testable.
| const values: string[] = []; | ||
| let present = false; | ||
| for (const key of [ | ||
| 'mentions', |
There was a problem hiding this comment.
[Suggestion — code quality + future maintainer] collectMentionValues checks 7 mention field names including open_id and openId (Feishu-specific, never appear in WeCom payloads) and 5 item key names. Only mentions with userid is exercised in tests.
Two concerns:
- Dead code:
open_id/openIdare Feishu-specific. Trim to documented WeCom API field names. - Silent mention failure: if
collectMentionValuesfinds no recognized fields,isMentioneddefaults tofalsefor group chats — all group mentions silently drop with no log entry. When WeCom adds a new mention field format, the adapter appears healthy but is deaf to mentions.
Suggested: log a diagnostic when no mention fields are recognized in group context:
if (!mentions.present && isGroup) {
log(`no recognized mention fields in payload; mention detection may be outdated`);
}|
|
||
| function resolveConfigEnvVar(value: string, mode: EnvResolution): string { | ||
| if (mode === false) return value; | ||
| if (mode === 'available' && value.startsWith('$')) { |
There was a problem hiding this comment.
[Suggestion — future maintainer] resolveConfigEnvVar has three modes (true, false, 'available') but the semantic difference is undocumented and nearly invisible. Both true and 'available' resolve $-prefixed values and throw if the env var is missing. The only difference is error message quality.
A future maintainer adding a new parseChannelConfig call site will guess at which mode to use. The name 'available' suggests "only resolve if available" but it actually means "resolve $-prefixed values with explicit not-set vs empty diagnostics" — the opposite of what the name implies.
Suggested: add a JSDoc block on the EnvResolution type explaining each mode, or collapse true and 'available' into a single path.
| ): Record<string, unknown> | undefined { | ||
| return asRecord(value?.[key]); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Nice to have] sanitizeFileName does not bound output length. A crafted Content-Disposition header or WeCom payload with a very long filename field produces a sanitized name that can exceed the filesystem's 255-byte component limit, causing writeFile to fail with ENAMETOOLONG.
Not a security issue (the error is caught), but an availability concern. Simple fix: .slice(0, 200) after sanitization.
| if (embedded) return isPublicIpv4(embedded); | ||
| const groups = expandIpv6Groups(host); | ||
| if (!groups) return false; | ||
| const first = groups[0] ?? 0; |
There was a problem hiding this comment.
[Nice to have] ~177 lines of hand-rolled IPv4/IPv6 address classification (isPublicIpAddress, parseIpv4Parts, parseMappedIpv4, parseEmbeddedIpv4, expandIpv6Groups, normalizeIpv6DottedSuffix, etc.). This is security-critical code (SSRF prevention).
parseIpv4Parts uses Number(part) which silently accepts leading zeros (010 → 10), scientific notation (1e2 → 100), and empty strings ('' → 0). Safe today because isIP() pre-validates in isPublicIpAddress, but fragile if refactored.
Consider using ipaddr.js (47KB, zero deps, widely used): ipaddr.parse(addr).range() === 'unicast' covers all edge cases in one call. If adding a dependency is unacceptable, at minimum add strict format validation to parseIpv4Parts (reject leading zeros, non-digit chars, empty parts).
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
| if (this.kickReconnectRetry === undefined) { | ||
| this.scheduleKickReconnectRetry( | ||
| reason, | ||
| this.disconnectGeneration, |
There was a problem hiding this comment.
[Critical] The catch handler's fallback scheduleKickReconnectRetry does not pass resetRetryCycles: true. If reconnectAfterKick throws when kickReconnectRetryCycles is already at KICK_RECONNECT_MAX_RETRY_CYCLES (e.g., the exhaustion branch itself throws before scheduling its own long retry), the adapter enters a stuck loop: the 15-minute timer fires, resets kickReconnectAttempts but not kickReconnectRetryCycles, calls reconnectAfterKick which immediately hits exhaustion again, schedules another long retry (with resetRetryCycles: true this time), but by then cycles has been incremented again — each recovery attempt takes 15+ minutes.
| this.disconnectGeneration, | |
| this.scheduleKickReconnectRetry( | |
| reason, | |
| this.disconnectGeneration, | |
| KICK_RECONNECT_LONG_RETRY_MS, | |
| reconnectReason, | |
| true, | |
| ); |
— qwen3.7-max via Qwen Code /review
| new Error('WeCom SDK activity watchdog timed out.'), | ||
| 'activity watchdog', | ||
| ); | ||
| }, ACTIVITY_WATCHDOG_INTERVAL_MS); |
There was a problem hiding this comment.
[Critical] setInterval is missing .unref?.(). Every other timer in this file (dedupTimer, kickReconnectReset, kickReconnectRetry, disconnectReconnectFallback, authentication timeouts) consistently calls .unref?.() to avoid keeping the Node.js event loop alive. The activity watchdog interval is the sole exception.
Without .unref(), the 60-second repeating interval keeps the event loop alive indefinitely — the CLI process will hang on shutdown when the WeCom channel is connected.
| }, ACTIVITY_WATCHDOG_INTERVAL_MS); | |
| }, ACTIVITY_WATCHDOG_INTERVAL_MS); | |
| this.activityWatchdog.unref?.(); | |
| } |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
/review Summary — PR #6224: feat(channels): add WeCom intelligent robot channel
Verdict: ✅ Approve
Scope reviewed
- 23 files, +7174 / −25 lines
- Core adapter:
WeComAdapter.ts(~2100 lines) — connect/disconnect lifecycle, message handling, media upload/download, SSRF protection, markdown chunking, dedup, reconnection - Base class refactoring:
ChannelBase.ts— preflightInbound/processInbound split, collect buffer lifecycle hooks - Config utilities:
config-utils.ts—EnvResolutiontype,resolveConfigEnvVarwith'available'mode - Test suite: 129 tests covering reconnection, SSRF, media handling, dedup, lifecycle hooks
Review methodology
- 9 parallel analysis agents: security/SSRF, lifecycle hooks, message dedup, media handling, markdown chunking, reconnection state machine, config utilities, cross-file impact, test coverage
- Verification round: 4 candidate findings investigated; 3 rejected (already addressed or not real), 1 confirmed low-confidence
- Reverse audit: additional pass looking for missed issues — none found
- Deduplication: all findings checked against 30 existing review comments from 12+ prior automated review rounds
Key areas assessed
| Area | Assessment |
|---|---|
| SSRF protection (DNS rebinding, IP classification) | Robust — IPv4/IPv6 private/reserved ranges correctly enumerated, DNS rebinding mitigated via post-resolution re-check |
| Message deduplication (seenMessages + inFlightMessages) | Sound two-layer tracking with TTL-based cleanup |
| Lifecycle hooks (onPromptBuffered/Drained/Dropped) | Consistent with ChannelBase contract, WeakSet guard prevents double-processing |
| Markdown chunking (4096-byte limit) | Handles CJK multi-byte boundaries correctly |
| Reconnection state machine | Exponential backoff with jitter, proper cleanup on disconnect |
| Media attachment handling | Size caps enforced, MIME detection via magic bytes, temp directory cleanup |
| Config env var resolution | 'available' mode correctly distinguishes explicit unset from empty string |
| Cross-file impact on ChannelBase consumers | preflightInbound split is backward-compatible — existing adapters unaffected |
Findings
No new high-confidence Critical or Suggestion findings beyond what prior review rounds have already covered. All candidate issues investigated during this review were either:
- Already discussed and addressed in prior rounds
- Verified as non-issues through code analysis and test execution
- Below the confidence threshold for actionable feedback
— qwen3.7-max via Qwen Code /review
|
Closing this PR and replacing it with a fresh PR after a local self-review and verification pass. The old thread has accumulated a large amount of outdated review history, which makes the current review state hard to read. Local verification on head
I also re-checked the latest critical review areas around reconnect retry/reset behavior, preflight/process gating, SDK logging, media URL safety, outbound media restrictions, and env-var resolution before opening the replacement. |
3 participants
What this PR does
Rewrites the WeCom channel implementation to use WeCom intelligent robot API mode instead of a self-built application callback. The built-in
wecomchannel now connects through the official@wecom/aibot-node-sdkWebSocket client with Bot ID and Secret, so users do not need Corp ID, Agent ID, callback Token, EncodingAESKey, or a public callback URL.The channel receives direct and group messages through the long connection, normalizes text, voice transcription, mixed text plus image, image, file, video, and quoted content into the existing channel flow, and uses the shared channel base for allowlist, pairing, open mode, group policy, session routing, and attachment handling. Responses are sent as WeCom markdown, with local image/file/video/voice media markers uploaded through the SDK when the generated path is inside the workspace or system temporary directory.
The PR also registers
wecomas a built-in channel type, adds documentation for intelligent robot setup and Bot ID/Secret configuration, and updates channel config parsing so plugin-required credentials such asbotIdandsecretare type-checked and can be read from environment variables.Why it's needed
The WeCom intelligent robot setup flow exposes Bot ID and Secret, and it uses an outbound WebSocket connection. That is the setup users see in the current WeCom docs and is simpler than a self-built application callback deployment.
Reviewer Test Plan
How to verify
Create a WeCom intelligent robot in API mode, configure a channel with
type: "wecom",botId,secret, the desired sender policy, and a projectcwd, then runqwen channel start my-wecom. Send direct and group messages to the robot and confirm that direct messages work, group messages respect the configured group policy, and the configured sender policy is enforced.Send text, voice, image, mixed text plus image, file, and video messages to confirm the agent receives text and attachments. Ask the agent to return markdown text and, for local media generated inside the configured workspace or temp directory, include
[IMAGE: ...],[FILE: ...],[VIDEO: ...], or[VOICE: ...]markers to confirm outbound media upload.Local verification completed:
cd packages/channels/wecom && npx vitest run src/WeComAdapter.test.ts;cd packages/cli && npx vitest run src/commands/channel/config-utils.test.ts;npm run build;npm run typecheck.Evidence (Before & After)
Before this rewrite, the PR described and implemented a WeCom self-built application callback path that required Corp ID, Agent ID, callback Token, EncodingAESKey, and webhook forwarding. After this rewrite,
wecomuses WeCom intelligent robot API mode with Bot ID and Secret over a long-lived WebSocket connection.Tested on
Risk & Scope
wecomwas not previously a built-in released channel.Linked Issues
Fixes #6208