feat(channels): add WeCom intelligent robot channel#6210
Conversation
|
Thanks for the PR, @qqqys! Template looks good ✓ — all required sections present with bilingual translation. On direction: adding WeCom (Enterprise WeChat) as a built-in channel is a natural fit alongside the existing telegram, dingtalk, feishu, and qqbot adapters. Teams using WeCom are a real audience, and the On approach: the scope feels right for a first slice — text-only callbacks, no external XML dependency, clean separation of adapter / callback / crypto. One thing that needs fixing before merge:
Moving on to code review and testing. 🔍 中文说明感谢贡献，@qqqys！ 模板完整 ✓ — 所有必需章节齐全，含双语翻译。 方向：将 WeCom（企业微信）作为内置 channel 加入，与现有的 telegram、dingtalk、feishu、qqbot 适配器一致，是自然的扩展。使用企业微信的团队是真实受众， 方案：首版范围合理 — 仅支持文本回调，未引入外部 XML 依赖，adapter / callback / crypto 分离清晰。合并前需修复一个问题：
进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent baseline: for a new WeCom channel, I'd expect a package extending The PR's implementation matches this closely. The code is well-structured across four source files (
No critical blockers or AGENTS.md violations found in the source code. The TestingWeCom unit tests (4 test files, 22 tests — all pass)CLI config-utils tests (1 file, 23 tests — all pass, including new wecom validation test)Build + TypecheckBoth pass with 0 errors. Channel registryWeCom is registered as a built-in channel type alongside dingtalk, feishu, and qqbot. 中文说明代码审查独立基线：新的 WeCom channel 需要一个扩展 PR 的实现与此一致。代码结构清晰，四个源文件（
源码中未发现关键阻塞问题或 AGENTS.md 违规。 Stage 1 提到的 测试
— Qwen Code · qwen3.7-max |
|
Stepping back: this is a solid channel adapter PR. The implementation follows the established pattern from dingtalk/feishu/qqbot, the crypto is correct, token management handles edge cases well, and test coverage is thorough (22 tests across crypto, callback parsing, adapter behavior, config validation, and plugin registration). The scope is right for a first slice — text-only, no speculative features. My independent proposal matched the PR's approach closely. I don't see a materially simpler path — a WeCom channel adapter needs callback verification, AES decryption, XML parsing, HTTP serving, and outbound API calls, and this PR covers all of that without over-engineering. The only issue is the committed Approving. ✅ 中文说明总体来看：这是一个扎实的 channel 适配器 PR。实现遵循了 dingtalk/feishu/qqbot 的既有模式，加解密正确，token 管理处理了边界情况，测试覆盖充分（22 个测试覆盖加解密、回调解析、适配器行为、配置校验和插件注册）。首版范围合理 — 仅文本，无投机性功能。 我的独立方案与 PR 的方法高度一致。没有看到明显更简的路径 — WeCom channel 适配器需要回调校验、AES 解密、XML 解析、HTTP 服务和出站 API 调用，PR 全部覆盖且没有过度工程。 唯一的问题是提交的 批准 ✅ — 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. ✅ Just one minor cleanup: please remove the committed dist/ files (git rm --cached -r packages/channels/wecom/dist/).
| @@ -0,0 +1,242 @@ | |||
| import { createServer } from 'node:http'; | |||
There was a problem hiding this comment.
[Critical] Committed build artifacts (dist/ directory)
The PR includes 12 compiled files under packages/channels/wecom/dist/ (.js, .d.ts, .js.map). No other channel package (dingtalk, feishu, qqbot, telegram, weixin) commits dist/ to git — the root .gitignore excludes dist. These files are generated by tsc --build and should only exist in CI/release builds.
This inflates the PR by ~460 lines of generated code, creates maintenance burden (every source change requires re-committing dist), and breaks the convention every other channel follows.
git rm -r packages/channels/wecom/dist/— qwen3.7-max via Qwen Code /review
| if (!text.trim()) return; | ||
| for (const chunk of splitByUtf8Bytes(text, MAX_TEXT_BYTES)) { | ||
| const token = await this.getAccessToken(); | ||
| const resp = await fetch(`${SEND_API}?access_token=${token}`, { |
There was a problem hiding this comment.
[Critical] fetch() calls lack AbortSignal.timeout() — can hang indefinitely
Both fetchAccessToken() and sendMessage() use bare fetch() with no timeout. If the WeCom API hangs (DNS timeout, TCP blackhole, server-side hang), these calls block indefinitely. A hung token fetch also blocks all future sendMessage calls because getAccessToken() awaits the same deduplicated promise.
Every other similar adapter uses timeouts: DingTalk uses AbortSignal.timeout(PROACTIVE_FETCH_TIMEOUT_MS), Feishu uses AbortSignal.timeout(15_000).
| const resp = await fetch(`${SEND_API}?access_token=${token}`, { | |
| const resp = await fetch(`${SEND_API}?access_token=${encodeURIComponent(token)}`, { | |
| signal: AbortSignal.timeout(15_000), |
Also note: token should be URL-encoded for consistency with fetchAccessToken() which uses encodeURIComponent for its query parameters.
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| disconnect(): void { |
There was a problem hiding this comment.
[Critical] disconnect() doesn't force-close connections — EADDRINUSE on reconnect
server.close() stops accepting new connections but does NOT terminate existing keep-alive connections. Setting this.server = undefined immediately means a subsequent connect() call creates a new server on the same port while the old one is still draining — causing EADDRINUSE.
Compare with the Feishu adapter which calls this.httpServer.closeAllConnections() before close().
| disconnect(): void { | |
| disconnect(): void { | |
| if (this.server) { | |
| this.server.closeAllConnections(); | |
| this.server.close(); | |
| this.server = undefined; | |
| } | |
| process.stderr.write(`[WeCom:${this.name}] Disconnected.\n`); | |
| } |
— qwen3.7-max via Qwen Code /review
| this.wecom.corpId, | ||
| )}&corpsecret=${encodeURIComponent(this.wecom.secret)}`; | ||
| const resp = await fetch(url); | ||
| const data = (await resp.json()) as WeComTokenResponse; |
There was a problem hiding this comment.
[Suggestion] fetchAccessToken() missing .catch() on resp.json() — confusing error on non-JSON response
If the WeCom token API returns a non-JSON response (e.g., an HTML 502 page during an outage), resp.json() throws a raw SyntaxError: Unexpected token '<' instead of the structured error message.
sendMessage() at line 112 already handles this correctly with .catch(() => ({})) — apply the same pattern here:
| const data = (await resp.json()) as WeComTokenResponse; | |
| const data = (await resp.json().catch(() => ({}))) as WeComTokenResponse; |
— qwen3.7-max via Qwen Code /review
| return undefined; | ||
| } | ||
|
|
||
| const senderId = getXmlField(xml, 'FromUserName'); |
There was a problem hiding this comment.
[Suggestion] buildWeComEnvelope parses the full XML 4 times — once per getXmlField call
Each getXmlField call invokes parseWeComXmlFields(xml) which re-parses the entire XML from scratch. Parse once and reuse the map:
| const senderId = getXmlField(xml, 'FromUserName'); | |
| export function buildWeComEnvelope( | |
| channelName: string, | |
| xml: string, | |
| ): Envelope | undefined { | |
| const fields = parseWeComXmlFields(xml); | |
| if (fields.get('MsgType') !== 'text') { | |
| return undefined; | |
| } | |
| const senderId = fields.get('FromUserName'); | |
| const text = fields.get('Content'); | |
| if (!senderId || text === undefined) { | |
| return undefined; | |
| } | |
| return { | |
| channelName, | |
| senderId, | |
| senderName: senderId, | |
| chatId: senderId, | |
| text, | |
| messageId: fields.get('MsgId'), | |
| isGroup: false, | |
| isMentioned: false, | |
| isReplyToBot: false, | |
| }; | |
| } |
This also requires exporting parseWeComXmlFields (or inlining it).
— qwen3.7-max via Qwen Code /review
| touser: chatId, | ||
| msgtype: 'text', | ||
| agentid: this.wecom.agentId, | ||
| text: { content: chunk }, |
There was a problem hiding this comment.
[Suggestion] sendMessage doesn't invalidate the cached token on auth errors
When the WeCom API returns auth-related errcodes (40014 invalid access_token, 42001 expired), the stale token remains cached until its expiresAt time — up to ~2 hours of sustained message delivery failure.
Consider clearing the cache on auth errors:
if (!resp.ok || data.errcode !== 0) {
if (data.errcode === 40014 || data.errcode === 42001) {
this.accessToken = undefined;
}
throw new Error(...);
}— qwen3.7-max via Qwen Code /review
| @@ -0,0 +1,8 @@ | |||
| import { defineConfig } from 'vitest/config'; | |||
There was a problem hiding this comment.
[Suggestion] Missing resolve.alias for @qwen-code/channel-base
Both dingtalk/vitest.config.ts and qqbot/vitest.config.ts include a resolve.alias mapping @qwen-code/channel-base to its live source (../base/src/index.ts). This lets tests run without a prior tsc --build of base.
Without this alias, cd packages/channels/wecom && npx vitest run will fail from a clean checkout unless base has been built first.
| import { defineConfig } from 'vitest/config'; | |
| import path from 'node:path'; | |
| import { defineConfig } from 'vitest/config'; | |
| export default defineConfig({ | |
| test: { | |
| globals: true, | |
| }, | |
| resolve: { | |
| alias: { | |
| '@qwen-code/channel-base': path.resolve( | |
| __dirname, | |
| '../base/src/index.ts', | |
| ), | |
| }, | |
| }, | |
| }); |
— qwen3.7-max via Qwen Code /review
| const corpId = requiredString(name, config, 'corpId'); | ||
| const agentIdValue = config['agentId']; | ||
| const agentIdRaw = | ||
| typeof agentIdValue === 'number' |
There was a problem hiding this comment.
[Suggestion] Double env-var resolution for $-prefixed config values
The CLI's parseChannelConfig already resolves $-prefixed values for standard fields (like token) via resolveOptionalStringField → resolveEnvVars. The adapter then resolves again via requiredString → resolveEnvValue. Two risks:
- If a resolved env var's value itself starts with
$(e.g.,WECOM_TOKEN=$inner), the adapter's second pass attempts unintended double resolution. - A user whose literal config value starts with
$(e.g.,secret: '$ecureP@ss') gets a confusing "Environment variable ecureP@ss is not set" error.
Consider either removing resolveEnvValue from the adapter (relying on the CLI's resolution) or adding a $$ escape for literal dollar signs. If keeping it, the error message should include the channel name and field for debuggability.
— qwen3.7-max via Qwen Code /review
| const envelope = buildWeComEnvelope(this.name, xml); | ||
| if (envelope) { | ||
| this.handleInbound(envelope).catch((err: unknown) => { | ||
| process.stderr.write( |
There was a problem hiding this comment.
[Suggestion] Non-text messages are silently dropped with no logging
When buildWeComEnvelope returns undefined (for image, voice, video, location, or event messages), the callback responds with 200/success but produces no log entry. Users sending non-text messages to the bot see no response, and operators have no visibility into what's being discarded.
Consider adding a one-line notice:
if (envelope) {
this.handleInbound(envelope).catch(...);
} else {
process.stderr.write(
`[WeCom:${this.name}] Ignoring non-text callback.\n`,
);
}— 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 review findings to report. Downgraded from Approve to Comment: CI still running.
The WeCom channel adapter is well-structured and follows established conventions from other channel adapters (dingtalk/feishu/qqbot). Crypto implementation (AES-256-CBC, SHA1 with timingSafeEqual, PKCS#7 padding validation, corpId binding) is correct. Token caching/coalescing logic is sound. Build passes and all 22 wecom + 23 config-utils tests pass.
Three low-confidence items for human review (not flagged as inline comments):
WeComAdapter.ts:87-89: Server error handler removed afterlisten()succeeds — post-startup server errors would be uncaughtWeComAdapter.ts:165-168:handleInbounderror log lackssenderId/messageIdcontextWeComAdapter.test.ts:sendMessageandfetchAccessTokenerror paths are untested
— qwen3.7-max via Qwen Code /review
| } | ||
| throw err; | ||
| } | ||
| const xml = decryptWeComCallback({ |
There was a problem hiding this comment.
[Critical] When decryptWeComCallback throws (bad signature, wrong encodingAesKey, malformed XML), the error propagates to the createServer catch handler which responds with HTTP 400. WeCom retries non-200 responses with exponential backoff (~7 retries). A misconfigured channel produces an amplifying cascade of 400 responses — each WeCom retry triggers another decrypt failure and another 400.
For webhook receivers, unrecoverable crypto/config errors should return 200 to prevent the retry storm. Compare to how the Feishu adapter in this repo handles dispatch errors.
| const xml = decryptWeComCallback({ | |
| let xml: string; | |
| try { | |
| xml = decryptWeComCallback({ | |
| token: this.wecom.token, | |
| corpId: this.wecom.corpId, | |
| encodingAesKey: this.wecom.encodingAesKey, | |
| timestamp: requiredQuery(url, 'timestamp'), | |
| nonce: requiredQuery(url, 'nonce'), | |
| signature: requiredQuery(url, 'msg_signature'), | |
| body, | |
| }); | |
| } catch (err) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] decrypt/verify failed: ${sanitizeLogText(String(err), 200)}\n`, | |
| ); | |
| res.statusCode = 200; | |
| res.end('success'); | |
| return; | |
| } |
— qwen3.7-max via Qwen Code /review
| this.accessToken = undefined; | ||
| } | ||
| throw new Error( | ||
| `WeCom send failed: HTTP ${resp.status} errcode=${data.errcode ?? 'unknown'} ${sanitizeLogText( |
There was a problem hiding this comment.
[Suggestion] When text exceeds MAX_TEXT_BYTES and is split into N chunks, failure on chunk K (e.g., token expiry mid-send) drops chunks K+1 through N silently — no log entry indicating partial delivery occurred, no retry of the failed chunk after token refresh. The recipient receives a truncated message with no indication anything went wrong.
Consider logging which chunk failed and how many were dropped:
process.stderr.write(
`[WeCom:${this.name}] partial delivery: chunk ${chunkIndex}/${totalChunks} failed, ${totalChunks - chunkIndex} chunk(s) dropped\n`,
);Alternatively, retry the failed chunk once after clearing the cached token.
Also: this.getAccessToken() is awaited on every iteration but the token is the same for all chunks — consider fetching once before the loop.
— qwen3.7-max via Qwen Code /review
| const chunks: string[] = []; | ||
| let current = ''; | ||
| let currentBytes = 0; | ||
| for (const char of text) { |
There was a problem hiding this comment.
[Suggestion] for (const char of text) iterates by Unicode code point, not grapheme cluster. ZWJ emoji sequences (family emoji 👨👩👧👦 = 7 code points / ~25 bytes, flag sequences, skin-tone modifiers) can be split mid-sequence when they straddle the 1800-byte boundary. Each fragment is sent as a separate WeCom message with a broken partial glyph.
Use Intl.Segmenter (available in Node 22+) to iterate by grapheme cluster:
const segmenter = new Intl.Segmenter();
for (const { segment } of segmenter.segment(text)) {
const segBytes = Buffer.byteLength(segment);
if (current && currentBytes + segBytes > maxBytes) {
chunks.push(current);
current = '';
currentBytes = 0;
}
current += segment;
currentBytes += segBytes;
}— qwen3.7-max via Qwen Code /review
| @@ -0,0 +1,444 @@ | |||
| import { describe, expect, it, vi, afterEach } from 'vitest'; | |||
There was a problem hiding this comment.
[Suggestion] Several code paths in the adapter lack test coverage:
decryptWeComEcho: zero direct tests (only tested indirectly via the GET echo integration test)extractWeComEncryptmissing-<Encrypt>branch: no test with XML lacking the Encrypt fieldbuildWeComEnvelopewith missingFromUserNameorContent: no test for the!senderId || text === undefinedearly returnsendMessageempty/whitespace text: no test thatfetchis not calledsplitByUtf8Bytesmulti-byte boundary: only ASCII chunking tested; no CJK/emoji boundary testconnect()listen error (EADDRINUSE): the cleanup branch (this.server = undefined) is untestedrequiredQuerymissing-param: no test for the throw branch
— qwen3.7-max via Qwen Code /review
| agentId, | ||
| secret: requiredString(name, config, 'secret'), | ||
| token: requiredString(name, config, 'token'), | ||
| encodingAesKey: requiredString(name, config, 'encodingAesKey'), |
There was a problem hiding this comment.
[Suggestion] encodingAesKey is validated only as a non-empty string here. The 43-character regex and base64-decode length check happen in decodeEncodingAesKey (crypto.ts), which is first called during the first callback decryption. A misconfigured key (typo, truncated copy) passes startup, the channel binds its port, and the operator only discovers the error when the first callback fails with a cryptic decryption error.
Validate eagerly at config time:
| encodingAesKey: requiredString(name, config, 'encodingAesKey'), | |
| encodingAesKey: (() => { | |
| const key = requiredString(name, config, 'encodingAesKey'); | |
| if (!/^[A-Za-z0-9+/]{43}$/.test(key)) { | |
| throw new Error(`Channel "${name}" field "encodingAesKey" must be 43 base64 characters.`); | |
| } | |
| return key; | |
| })(), |
— qwen3.7-max via Qwen Code /review
|
|
||
| const valueStart = openEnd + 1; | ||
| const closeTag = `</${tag}>`; | ||
| if (xml.startsWith('<![CDATA[', valueStart)) { |
There was a problem hiding this comment.
[Suggestion] When <![CDATA[ is detected at valueStart but ]]> is NOT immediately followed by the expected </tag> close tag (malformed XML), the code falls through to the non-CDATA indexOf path. That path searches from valueStart, which still points at the <![CDATA[ marker — so it can match a close-tag substring that appears inside the CDATA content, extracting an incorrect value that includes the raw CDATA markers.
Fix: skip the field on malformed CDATA instead of falling through:
if (xml.startsWith('<![CDATA[', valueStart)) {
const cdataEnd = xml.indexOf(']]>', valueStart + 9);
if (cdataEnd === -1) break;
const closeStart = cdataEnd + 3;
if (xml.startsWith(closeTag, closeStart)) {
fields.set(tag, xml.slice(valueStart + 9, cdataEnd));
cursor = closeStart + closeTag.length;
} else {
cursor = closeStart; // malformed CDATA; skip field
}
continue;
}— 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 (independent baseline)
Overall this is a well-structured WeCom channel adapter that follows the established conventions from dingtalk/feishu/qqbot. The crypto implementation is correct, the token management handles edge cases well, and test coverage is comprehensive.
I found 3 actionable suggestions and several minor observations. None are blocking — this looks close to merge-ready.
| } | ||
|
|
||
| export function decryptWeComCallback(input: DecryptWeComCallbackInput): string { | ||
| const encrypt = extractWeComEncrypt(input.body); |
There was a problem hiding this comment.
[Suggestion] XML parsing runs before signature verification
extractWeComEncrypt calls getXmlField → parseWeComXmlFields which fully parses the raw XML body before verifyWeComSignature validates the SHA1 signature below. An attacker can send a crafted, very large or complex XML body that burns CPU in the XML parser before the signature check rejects it.
Suggested fix: extract the <Encrypt> value with a lightweight substring search first (no full parse), verify the signature, then do the full XML parse only on the decrypted plaintext:
export function extractWeComEncrypt(encryptedXml: string): string {
const openTag = '<Encrypt><![CDATA[';
const closeTag = ']]></Encrypt>';
const start = encryptedXml.indexOf(openTag);
if (start === -1) throw new Error('WeCom callback is missing Encrypt.');
const valueStart = start + openTag.length;
const end = encryptedXml.indexOf(closeTag, valueStart);
if (end === -1) throw new Error('WeCom callback is missing Encrypt.');
return encryptedXml.slice(valueStart, end);
}Then in decryptWeComCallback, call extractWeComEncrypt (fast), verify signature, then full-parse the decrypted plaintext for buildWeComEnvelope (already trusted).
| ); | ||
| } | ||
|
|
||
| async sendMessage(chatId: string, text: string): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] No retry after token invalidation — message can be silently lost
When the WeCom API returns auth errcodes (40014 / 42001), the token is correctly invalidated (this.accessToken = undefined), but the current chunk throws immediately. For multi-chunk messages, if chunk K fails with a token error, chunks K+1 through N are silently dropped.
Suggested fix: add a single retry on token-invalidation errcodes:
for (const chunk of splitByUtf8Bytes(text, MAX_TEXT_BYTES)) {
let attempts = 0;
let sent = false;
while (!sent && attempts < 2) {
const token = await this.getAccessToken();
// ... fetch ...
if (data.errcode && INVALID_TOKEN_ERRCODES.has(data.errcode)) {
this.accessToken = undefined;
attempts++;
continue; // retry with fresh token
}
sent = true;
// ... existing error handling ...
}
}| } | ||
|
|
||
| async connect(): Promise<void> { | ||
| if (this.server) return; |
There was a problem hiding this comment.
[Suggestion] HTTP server has no request timeout — slow-loris risk
The createServer instance has no server.timeout, server.requestTimeout, or server.keepAliveTimeout configured. While the server binds to localhost (127.0.0.1) which limits the attack surface, an attacker with local access or via a forwarded port could hold connections open indefinitely.
Suggested fix after createServer:
this.server.timeout = 30_000;
this.server.requestTimeout = 15_000;
this.server.keepAliveTimeout = 5_000;
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured WeCom channel adapter following established dingtalk/feishu/qqbot patterns. Crypto, token management, and HTTP server hardening are all solid. Build passes, all 76 tests pass.
One critical gap: no message deduplication — WeCom retries deliver duplicate callbacks and this adapter forwards each one to handleInbound without a seenMessages guard (unlike dingtalk/feishu), causing duplicate bot responses.
Plus 5 actionable suggestions (misleading log, chunk abort, grapheme splitting, XML parser fragility, regex allows -).
See inline comments for details.
— Qwen Code /review
| } | ||
| const envelope = buildWeComEnvelope(this.name, xml); | ||
| if (envelope) { | ||
| this.handleInbound(envelope).catch((err: unknown) => { |
There was a problem hiding this comment.
[Critical] No message deduplication — duplicate bot responses on WeCom retry
WeCom retries failed callbacks with exponential backoff, delivering the same MsgId multiple times. Both dingtalk and feishu adapters maintain a seenMessages map (keyed on messageId, with TTL-based expiry) to suppress duplicates before calling handleInbound.
This adapter calls this.handleInbound(envelope) directly, so every retry produces a duplicate bot response.
Suggested fix: add a private seenMessages = new Map<string, number>() field and check before handleInbound:
const msgId = envelope.messageId;
if (msgId) {
const seen = this.seenMessages.get(msgId);
if (seen && Date.now() - seen < 60_000) return;
this.seenMessages.set(msgId, Date.now());
}Also consider a periodic cleanup interval to prevent unbounded growth (dingtalk uses a 5-minute sweep).
— Qwen Code /review
| }); | ||
| } else { | ||
| process.stderr.write( | ||
| `[WeCom:${this.name}] Ignoring non-text callback.\n`, |
There was a problem hiding this comment.
[Suggestion] Misleading log message
"Ignoring non-text callback." is logged for all undefined returns from buildWeComEnvelope, but that function also returns undefined for malformed text messages (missing <FromUserName> or <Content>). A message like "Ignoring unsupported or malformed callback." would be more accurate and help with debugging.
— Qwen Code /review
| continue; | ||
| } | ||
| } | ||
| throw new Error( |
There was a problem hiding this comment.
[Suggestion] sendMessage aborts all remaining chunks on failure
When chunk K fails (e.g. network error, non-token errcode), the throw on line 146 breaks out of both the retry loop and the outer chunk loop — chunks K+1…N are silently lost with no log indicating partial delivery occurred.
Consider either:
- Logging the partial-delivery state before throwing (e.g.
"WeCom send aborted after chunk K/N") - Continuing to send remaining chunks and collecting errors for a summary throw at the end
— Qwen Code /review
| const chunks: string[] = []; | ||
| let current = ''; | ||
| let currentBytes = 0; | ||
| for (const char of text) { |
There was a problem hiding this comment.
[Suggestion] splitByUtf8Bytes can split ZWJ emoji mid-sequence
for (const char of text) iterates by Unicode code point, not grapheme cluster. ZWJ emoji sequences (e.g. 👨👩👧👦 = 7 code points / ~25 bytes, flag sequences, skin-tone modifiers) can be split across chunk boundaries, producing mojibake in WeCom.
Low priority since this mainly matters for emoji-heavy messages near the 1800-byte boundary. If you want to handle it, consider using Intl.Segmenter (Node 22+) or falling back to Array.from(new Intl.Segmenter().segment(text)) for grapheme-aware iteration.
— Qwen Code /review
| const openEnd = xml.indexOf('>', openStart + 1); | ||
| if (openEnd === -1) break; | ||
| const tag = xml.slice(openStart + 1, openEnd).trim(); | ||
| if (!/^[A-Za-z0-9_:-]+$/.test(tag) || tag === 'xml') { |
There was a problem hiding this comment.
[Suggestion] XML parser silently skips tags with attributes
The tag regex /^[A-Za-z0-9_:-]+$/ rejects any tag containing attributes (e.g. <Content type="text">), causing the field to be silently dropped. While WeCom's current callback schema doesn't use attributes, this is fragile against future schema changes or edge-case payloads.
Consider stripping attributes after extracting the tag name:
const rawTag = xml.slice(openStart + 1, openEnd).trim();
const spaceIdx = rawTag.indexOf(' ');
const tag = spaceIdx === -1 ? rawTag : rawTag.slice(0, spaceIdx);— Qwen Code /review
| } | ||
|
|
||
| function decodeEncodingAesKey(encodingAesKey: string): Buffer { | ||
| if (!/^[A-Za-z0-9+/=-]{43}$/.test(encodingAesKey)) { |
There was a problem hiding this comment.
[Suggestion] encodingAesKey regex allows - character
The regex /^[A-Za-z0-9+/=-]{43}$/ accepts -, which is not part of standard base64 (only + and / are). The decoded-length check on line 67 catches some invalid inputs, but the regex should be tightened to /^[A-Za-z0-9+/=]{43}$/ to match the actual WeCom encoding spec.
— Qwen Code /review
| res.end('success'); | ||
| return; | ||
| } | ||
| const envelope = buildWeComEnvelope(this.name, xml); |
There was a problem hiding this comment.
[Critical] The decrypted callback is accepted without checking that the inner <AgentID> matches this channel's configured agentId. The PR already treats agentId as required config and uses it for outbound replies, but inbound text from any validly decrypted callback is routed into handleInbound; a shared callback route or reused Token/EncodingAESKey can therefore drive this channel from the wrong WeCom application.
Please parse AgentID from the decrypted XML and ack/drop callbacks whose value does not match String(this.wecom.agentId), with a regression test for a mismatched AgentID.
— GPT-5 Codex via Qwen Code /review
| typeof agentIdValue === 'number' | ||
| ? String(agentIdValue) | ||
| : requiredString(name, config, 'agentId'); | ||
| const agentId = Number(agentIdRaw); |
There was a problem hiding this comment.
[Suggestion] The numeric validation relies on raw Number() coercion, so malformed settings can silently become different values. For example, agentId: "0x10" or "1e3" is accepted, and webhookPort: true or [9322] becomes a valid port instead of failing fast.
Please validate the raw value type first, then accept only numbers or decimal digit strings before conversion.
— GPT-5 Codex via Qwen Code /review
| it('fetches an access token and sends a text application message', async () => { | ||
| const fetchMock = vi.fn(async (url: string, init?: RequestInit) => { | ||
| expect(init?.signal).toBeInstanceOf(AbortSignal); | ||
| if (url.includes('/gettoken')) { |
There was a problem hiding this comment.
[Suggestion] This branch treats any URL containing /gettoken as the token request, so the test would still pass if fetchAccessToken() dropped corpid, used the wrong query names, or stopped URL-encoding the credentials.
Please assert the exact gettoken URL, ideally using special characters in corpId or secret so encoding regressions are caught.
— GPT-5 Codex via Qwen Code /review
|
|
||
| await createChannel().sendMessage('zhangsan', 'a'.repeat(1901)); | ||
|
|
||
| expect(bodies).toHaveLength(2); |
There was a problem hiding this comment.
[Suggestion] The chunking test only checks the number of outbound messages and each chunk's max byte length. A splitter that duplicated, reordered, or dropped part of the reply could still satisfy those assertions.
Please collect each sent text.content and assert that chunks.join('') equals the original long reply.
— GPT-5 Codex via Qwen Code /review
| for (const chunk of splitByUtf8Bytes(text, MAX_TEXT_BYTES)) { | ||
| for (let attempt = 0; attempt < 2; attempt++) { | ||
| const token = await this.getAccessToken(); | ||
| const resp = await fetch( |
There was a problem hiding this comment.
[Suggestion] The timeout signals are in place, but network/proxy/DNS failures and AbortSignal.timeout() rejections escape before the contextual WeCom send failed / WeCom gettoken failed errors are built. Production logs can end up with only a raw TypeError or TimeoutError, without indicating which WeCom operation failed.
Please wrap the send and token-refresh fetch() calls at the operation boundary and rethrow sanitized contextual errors.
— GPT-5 Codex via Qwen Code /review
| res.end('success'); | ||
| return; | ||
| } | ||
| this.handleInbound(envelope).catch((err: unknown) => { |
There was a problem hiding this comment.
[Suggestion] No dedup rollback when handleInbound fails — silently drops retried messages
isDuplicateMessage() inserts the message ID into seenMessages synchronously before handleInbound() fires. If handleInbound throws (bridge offline, session error), the dedup entry is never removed. If the initial 200 response is lost at the TCP level and WeCom retries, the retry is incorrectly deduplicated — the user's message is silently lost.
The Feishu adapter explicitly handles this:
// FeishuAdapter.ts
this.handleInbound(envelope).catch((err) => {
this.seenMessages.delete(msgId); // Allow Feishu retries by removing the dedup entry on failure
...
});| this.handleInbound(envelope).catch((err: unknown) => { | |
| this.handleInbound(envelope).catch((err: unknown) => { | |
| if (envelope.messageId) { | |
| this.seenMessages.delete(envelope.messageId); | |
| } | |
| const message = envelope.messageId | |
| ? ` message=${sanitizeLogText(envelope.messageId, 80)}` | |
| : ''; | |
| process.stderr.write( | |
| `[WeCom:${this.name}] inbound handling failed for sender=${sanitizeLogText( | |
| envelope.senderId, | |
| 80, | |
| )}${message}: ${sanitizeLogText(String(err), 200)}\n`, | |
| ); | |
| }); |
— qwen3.7-max via Qwen Code /review
| if (!Number.isInteger(agentId) || agentId <= 0) { | ||
| throw new Error(`Channel "${name}" field "agentId" must be a number.`); | ||
| } | ||
| const webhookPort = Number(config['webhookPort']); |
There was a problem hiding this comment.
[Suggestion] webhookPort bypasses env-var resolution unlike every other config field
Every other field (corpId, agentId, secret, token, encodingAesKey) goes through requiredString → resolveEnvValue. webhookPort is read via Number(config['webhookPort']) directly. If a user writes "webhookPort": "$WECOM_WEBHOOK_PORT" following the env-var pattern, they get NaN and a misleading error: field "webhookPort" must be a valid TCP port — with no indication the $-prefixed value was treated as a literal string.
| const webhookPort = Number(config['webhookPort']); | |
| const rawPort = config['webhookPort']; | |
| const resolvedPort = typeof rawPort === 'string' ? resolveEnvValue(rawPort) : rawPort; | |
| const webhookPort = Number(resolvedPort); |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| const closeStart = xml.indexOf(closeTag, valueStart); | ||
| if (closeStart === -1) break; |
There was a problem hiding this comment.
[Suggestion] parseWeComXmlFields hard-aborts the entire parse on any unrecognized construct, silently dropping all subsequent fields
When a close tag is not found (closeStart === -1), break exits the while loop entirely. All fields after the problematic construct are silently discarded. The same break applies for unclosed CDATA (line 129) and unclosed open tags (line 117). In contrast, the tag-name validation path uses continue (skip one tag, keep parsing).
If WeCom adds a new self-closing or nested element before Content or MsgId in a future API revision, the parser aborts before reaching those fields, and buildWeComEnvelope returns undefined — producing the same "Ignoring unsupported or malformed callback" log used for routine image/voice skips.
| if (closeStart === -1) break; | |
| const closeStart = xml.indexOf(closeTag, valueStart); | |
| if (closeStart === -1) { | |
| cursor = valueStart; | |
| continue; | |
| } |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| function decodeEncodingAesKey(encodingAesKey: string): Buffer { | ||
| if (!/^[A-Za-z0-9+/=]{43}$/.test(encodingAesKey)) { |
There was a problem hiding this comment.
[Nice to have] encodingAesKey regex accepts +, /, = characters that WeCom never generates
WeCom's EncodingAESKey specification defines the key as exactly 43 characters from [a-zA-Z0-9] only. A config typo that introduces +, /, or = passes this regex, and the error surfaces later during AES decryption as an opaque "Invalid WeCom encrypted payload" — far from the actual misconfiguration.
The appended = on the next line provides the single padding character needed for a 32-byte decode, so the key itself should never contain padding.
| if (!/^[A-Za-z0-9+/=]{43}$/.test(encodingAesKey)) { | |
| if (!/^[A-Za-z0-9]{43}$/.test(encodingAesKey)) { |
— qwen3.7-max via Qwen Code /review
| `Channel "${name}" field "${field}" must be a decimal integer.`, | ||
| ); | ||
| } | ||
| return Number(resolved); |
There was a problem hiding this comment.
[Suggestion] requiredDecimalInteger() still converts decimal strings with Number(resolved) without checking that the parsed value is finite and safely representable. A very large decimal string passes the regex but becomes Infinity, so agentId passes the positive check and is serialized as null; large unsafe integers can also round and then fail inbound AgentID matching at runtime.
Because this can also affect numeric config values, parse into a local value and require Number.isSafeInteger(...) in both branches before returning.
const parsed = Number(resolved);
if (!Number.isSafeInteger(parsed)) {
throw new Error(
`Channel "${name}" field "${field}" must be a safe decimal integer.`,
);
}
return parsed;— GPT-5 Codex via Qwen Code /review
|
|
||
| private isDuplicateMessage(messageId: string): boolean { | ||
| const now = Date.now(); | ||
| for (const [id, timestamp] of this.seenMessages) { |
There was a problem hiding this comment.
[Suggestion] isDuplicateMessage() sweeps the entire seenMessages map on every callback before checking the current message ID. Under a burst of unique WeCom callbacks, the hot path becomes O(number of retained IDs) for the whole five-minute window. The adjacent DingTalk, Feishu, and QQBot adapters keep the per-message duplicate check O(1) and evict stale IDs from a periodic or lazy cleanup timer.
Move the TTL sweep out of this method: add a cleanup timer owned by the channel lifecycle, have isDuplicateMessage() only do has/set, and clear both the timer and map on disconnect().
private isDuplicateMessage(messageId: string): boolean {
if (this.seenMessages.has(messageId)) return true;
this.seenMessages.set(messageId, Date.now());
return false;
}— GPT-5 Codex 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.
Qwen Code Review — PR #6210
1 inline comment posted on specific code locations.
Needs human review: 2 low-confidence items (silent dedup logging, O(n) dedup sweep) — see inline context.
Overall: solid first contribution. The adapter is well-structured with good crypto hygiene (timingSafeEqual, PKCS#7 validation, base64 length checks). The most actionable finding is the missing supportsProactiveSend() override.
Review by qwen3.7-max via Qwen Code /review
| private serverErrorHandler?: (err: Error) => void; | ||
| private accessToken?: { value: string; expiresAt: number }; | ||
| private tokenRefresh?: Promise<string>; | ||
| private readonly seenMessages = new Map<string, number>(); |
There was a problem hiding this comment.
[Suggestion] Missing supportsProactiveSend() override.
ChannelBase returns false by default. WeCom clearly supports proactive sending — sendMessage() calls the WeCom message/send API with an access token. Without this override, loop and scheduled messages are rejected at runtime with "Channel does not support proactive loop messages."
All comparable channel adapters override this:
DingtalkAdapter:override supportsProactiveSend() { return true; }FeishuAdapter: sameTelegramAdapter: same
| private readonly seenMessages = new Map<string, number>(); | |
| override supportsProactiveSend(): boolean { | |
| return true; | |
| } | |
| } | |
| export class WeComChannel extends ChannelBase { |
— qwen3.7-max via Qwen Code /review
| const closeTag = `</${tag}>`; | ||
| if (xml.startsWith('<![CDATA[', valueStart)) { | ||
| const cdataEnd = xml.indexOf(']]>', valueStart + 9); | ||
| if (cdataEnd === -1) break; |
There was a problem hiding this comment.
[Critical] CDATA injection in Content can overwrite FromUserName — bypasses senderPolicy allowlist
parseWeComXmlFields uses fields.set(tag, value) (last-write-wins). Because indexOf(']]>', ...) finds the first ]]> in CDATA, a user message containing ]]> inside <Content><![CDATA[...]]></Content> causes the parser to close the CDATA early. Everything after the injected ]]> is then parsed as new XML tags. Since FromUserName precedes Content in standard WeCom callback XML, the real sender is set first and then overwritten by the attacker's injected <FromUserName> — bypassing senderPolicy: "allowlist" with no log trace.
| if (cdataEnd === -1) break; | |
| // Use first-write-wins to prevent field overwrite: | |
| if (!fields.has(tag)) { | |
| fields.set(tag, xml.slice(valueStart + 9, cdataEnd)); | |
| } |
Alternatively, use a proper XML parser (e.g., a SAX parser) instead of indexOf-based field extraction.
— qwen3.7-max via Qwen Code /review
| ); | ||
| if (req.method === 'GET') { | ||
| const echo = this.verifyEcho(url); | ||
| res.statusCode = 200; |
There was a problem hiding this comment.
[Critical] GET echo handler returns 400 on verification failure — breaks WeCom URL registration
this.verifyEcho(url) can throw (invalid signature, missing query params, decryption failure), and the error propagates to the outer createServer catch handler which returns HTTP 400. WeCom's callback URL verification protocol requires HTTP 200 — a 400 response causes WeCom to reject the callback URL registration, preventing the channel from ever being activated.
The POST handler correctly catches decrypt errors and returns 200 (lines 212-223). The GET handler should follow the same pattern:
| res.statusCode = 200; | |
| if (req.method === 'GET') { | |
| try { | |
| const echo = this.verifyEcho(url); | |
| res.statusCode = 200; | |
| res.end(echo); | |
| } catch (err) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] echo verification failed: ${sanitizeLogText(String(err), 200)}\n`, | |
| ); | |
| res.statusCode = 200; | |
| res.end(''); | |
| } | |
| return; | |
| } |
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| disconnect(): void { |
There was a problem hiding this comment.
[Suggestion] disconnect() doesn't clear seenMessages dedup map — stale entries survive reconnect
disconnect() calls server.closeAllConnections(), server.close(), and sets this.server = undefined, but never clears this.seenMessages. The feishu adapter (FeishuAdapter.ts:1790) explicitly calls this.seenMessages.clear() in its disconnect. On reconnect, stale entries from the previous session could suppress legitimate WeCom message retries within the 5-minute TTL window.
| disconnect(): void { | |
| this.server.closeAllConnections(); | |
| this.server.close(); | |
| this.server = undefined; | |
| this.seenMessages.clear(); |
— qwen3.7-max via Qwen Code /review
| headers: { 'Content-Type': 'application/json' }, | ||
| signal: AbortSignal.timeout(WECOM_FETCH_TIMEOUT_MS), | ||
| body: JSON.stringify({ | ||
| touser: chatId, |
There was a problem hiding this comment.
[Suggestion] sendMessage network errors bypass the retry loop — reply silently dropped on transient failure
If fetchWithContext throws (DNS failure, connection reset, AbortSignal.timeout() rejection), the exception propagates out of the retry loop immediately without retrying. The retry-once-on-invalid-token logic only handles HTTP-level responses, not network-level failures. A single transient network hiccup drops the bot's reply silently.
Consider wrapping the fetch call in a try/catch inside the retry loop to handle network errors with a short delay before retry.
— qwen3.7-max via Qwen Code /review
| `[WeCom:${this.name}] send aborted after chunk ${index + 1}/${chunks.length}.\n`, | ||
| ); | ||
| } | ||
| throw new Error( |
There was a problem hiding this comment.
[Suggestion] disconnect()/connect() race condition on port binding
server.close() is asynchronous (port release happens on a future tick), but this.server = undefined is set synchronously on the next line. A subsequent connect() call passes the if (this.server) return guard and creates a new server that may fail listen() with EADDRINUSE before the OS releases the port.
Consider storing the close promise on the instance and awaiting it at the start of connect().
— qwen3.7-max via Qwen Code /review
| process.stderr.write( | ||
| `[WeCom:${this.name}] inbound handling failed for sender=${sanitizeLogText( | ||
| envelope.senderId, | ||
| 80, |
There was a problem hiding this comment.
[Suggestion] Ambiguous log conflates 3 distinct silent-drop paths
buildWeComEnvelope returns undefined for three different reasons (AgentID mismatch, non-text MsgType, missing required fields), but all three produce the same log line: "Ignoring unsupported or malformed callback." The DingTalk adapter logs each drop reason separately, making it far easier to diagnose misconfigured multi-app deployments or unsupported content types at 3 AM.
Consider logging the specific drop reason (e.g., msgType, agentId match result) before the if (envelope) check.
— qwen3.7-max via Qwen Code /review
| return this.tokenRefresh; | ||
| } | ||
| this.tokenRefresh = this.fetchAccessToken().finally(() => { | ||
| this.tokenRefresh = undefined; |
There was a problem hiding this comment.
[Suggestion] Failed token fetches not cached — thundering herd on rate-limited API
When fetchAccessToken() rejects, the .finally() handler clears this.tokenRefresh immediately, so every subsequent caller triggers a fresh fetch with no backoff. Under burst traffic during a WeCom token API outage, this exhausts the rate-limited gettoken endpoint (300 calls/min per app). A short negative TTL (5-10 seconds) would prevent the thundering herd.
— qwen3.7-max via Qwen Code /review
| ); | ||
| const data = (await resp.json().catch(() => ({}))) as WeComApiResponse; | ||
| if (resp.ok && data.errcode === 0) { | ||
| break; |
There was a problem hiding this comment.
[Suggestion] No send-success audit trail — impossible to confirm reply delivery from logs
sendMessage logs failures but never logs success. The most common 3 AM question is "did the bot actually respond?" — with the current logging, you cannot answer this from stderr.
| break; | |
| if (resp.ok && data.errcode === 0) { | |
| if (index === chunks.length - 1) { | |
| process.stderr.write( | |
| `[WeCom:${this.name}] sent to=${sanitizeLogText(chatId, 80)} chunks=${chunks.length}.\n`, | |
| ); | |
| } | |
| break; | |
| } |
— qwen3.7-max via Qwen Code /review
| function resolveEnvValue(value: string): string { | ||
| if (!value.startsWith('$')) return value; | ||
| const envName = value.slice(1); | ||
| const resolved = process.env[envName]; |
There was a problem hiding this comment.
[Suggestion] resolveEnvValue has zero test coverage
This function handles $ENV_VAR interpolation for all sensitive config fields (corpId, secret, token, encodingAesKey) and throws when the env var is unset. Neither the happy path nor the error path has any test. Add tests that verify: (1) $MY_VAR resolves correctly when process.env.MY_VAR is set, and (2) $NONEXISTENT_VAR throws with the expected message.
— qwen3.7-max via Qwen Code /review
| export function extractWeComEncrypt(xml: string): string { | ||
| const openTag = '<Encrypt><![CDATA['; | ||
| const closeTag = ']]></Encrypt>'; | ||
| const start = xml.indexOf(openTag); |
There was a problem hiding this comment.
[Suggestion] CDATA asymmetry between extractWeComEncrypt and parseWeComXmlFields — hidden coupling
extractWeComEncrypt hard-requires CDATA wrapping (<Encrypt><![CDATA[...]]></Encrypt>) and rejects plain-text values. Meanwhile, parseWeComXmlFields handles both CDATA and bare text. A future refactor that unifies these two parsers would silently break one layer. Consider documenting the asymmetry or extracting a shared parser with a requireCdata flag.
— 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.
— qwen3.7-max via Qwen Code /review
| init: RequestInit, | ||
| context: string, | ||
| ): Promise<Response> { | ||
| try { |
There was a problem hiding this comment.
[Suggestion] fetchWithContext wraps errors with String(err), destroying the original error's .cause chain and stack trace.
Node.js undici distinguishes timeouts (AbortError), DNS failures (ENOTFOUND), connection resets (ECONNRESET), and TLS errors via err.code and err.cause.code. After String(err), all of these become the same opaque string, making it impossible to tell from logs whether a failure was a timeout, DNS issue, connection reset, or TLS error.
| try { | |
| async function fetchWithContext( | |
| url: string, | |
| init: RequestInit, | |
| context: string, | |
| ): Promise<Response> { | |
| try { | |
| return await fetch(url, init); | |
| } catch (err) { | |
| throw new Error(`${context}: ${sanitizeLogText(String(err), 200)}`, { cause: err }); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| }, | ||
| 'WeCom send failed', | ||
| ); | ||
| const data = (await resp.json().catch(() => ({}))) as WeComApiResponse; |
There was a problem hiding this comment.
[Suggestion] resp.json().catch(() => ({})) silently swallows non-JSON response bodies. When WeCom's API returns an HTML 502, plain-text error, or CAPTCHA page during outages, the actual diagnostic content is discarded and the thrown error shows errcode=unknown — which looks like a bug in the adapter rather than an upstream problem.
This applies to both this line (send API) and the fetchAccessToken method (token API, line 310).
| const data = (await resp.json().catch(() => ({}))) as WeComApiResponse; | |
| const rawBody = await resp.text(); | |
| let data: WeComApiResponse; | |
| try { | |
| data = JSON.parse(rawBody); | |
| } catch { | |
| throw new Error( | |
| `WeCom send failed: HTTP ${resp.status} non-JSON response: ${sanitizeLogText(rawBody, 200)}`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| process.stderr.write( | ||
| `[WeCom:${this.name}] send aborted after chunk ${index + 1}/${chunks.length}.\n`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The abort log includes the channel name and chunk position but omits chatId (the recipient). When multiple messages are being sent concurrently, there is no way to tell from logs whose message was partially delivered.
| } | |
| process.stderr.write( | |
| `[WeCom:${this.name}] send to ${sanitizeLogText(chatId, 80)} aborted after chunk ${index + 1}/${chunks.length}.\n`, | |
| ); |
— qwen3.7-max via Qwen Code /review
|
|
||
| constructor( | ||
| name: string, | ||
| config: ChannelConfig & Record<string, unknown>, |
There was a problem hiding this comment.
[Suggestion] seenMessages is a Map with no maximum size cap. Cleanup runs on a setInterval every DEDUP_TTL_MS (5 minutes). Between cleanups, a flood of unique message IDs causes unbounded memory growth — each entry is ~100+ bytes, so 100K unique messages in 5 minutes would hold ~10MB until the next timer tick.
Consider adding a maximum size check before inserting (e.g., evict oldest entries or skip dedup when the map exceeds a threshold), or switching to a TTL-based LRU cache.
— qwen3.7-max via Qwen Code /review
| if (this.server === server) { | ||
| this.server = undefined; | ||
| } | ||
| reject(err); |
There was a problem hiding this comment.
[Suggestion] The onError handler in connect() that handles listen failures (e.g., EADDRINUSE) is never exercised in tests. The mock listen always calls back successfully. Add a test where the mock server's listen triggers the once('error') handler instead of the callback, and assert the promise rejects, this.server is undefined afterward, and server.close() was called.
— qwen3.7-max via Qwen Code /review
| }, | ||
| 'WeCom gettoken failed', | ||
| ); | ||
| const data = (await resp.json().catch(() => ({}))) as WeComTokenResponse; |
There was a problem hiding this comment.
[Suggestion] The token API error test ("reports token API failures") uses HTTP 502, which fails at !resp.ok. No test covers when the token API returns HTTP 200 with errcode !== 0 (e.g., errcode: 40001 for invalid corpsecret), which is the far more common production failure mode. Add a test that mocks a 200 response with { errcode: 40001, errmsg: 'invalid corpsecret' } and asserts the error message includes errcode=40001.
— qwen3.7-max via Qwen Code /review
| break; | ||
| } | ||
| if (data.errcode && INVALID_TOKEN_ERRCODES.has(data.errcode)) { | ||
| this.accessToken = undefined; |
There was a problem hiding this comment.
[Suggestion] The retry test ("retries sends once after WeCom invalidates a cached token") only covers the success-after-retry path. No test verifies that when both attempts return an invalid-token errcode, the code correctly throws with the second attempt's response. Add a test where both calls return errcode: 42001 and assert the promise rejects with exactly 2 token fetches + 2 send calls.
— qwen3.7-max via Qwen Code /review
| `[WeCom:${this.name}] Ignoring unsupported or malformed callback.\n`, | ||
| ); | ||
| } | ||
| res.statusCode = 200; |
There was a problem hiding this comment.
[Suggestion] The res.statusCode = 405 branch for unsupported HTTP methods (PUT, DELETE, OPTIONS, etc.) has no test coverage. Add a test that sends a non-GET/non-POST request and asserts statusCode === 405 and the response body is 'method not allowed'.
— qwen3.7-max via Qwen Code /review
| return Buffer.concat(chunks).toString('utf8'); | ||
| } | ||
|
|
||
| function splitByUtf8Bytes(text: string, maxBytes: number): string[] { |
There was a problem hiding this comment.
[Suggestion] splitByUtf8Bytes is only tested with single-byte ASCII ('a'.repeat(1901)). WeCom is primarily used by Chinese-speaking users who send CJK text (3 bytes per character in UTF-8). Add a test with multi-byte characters — e.g., splitByUtf8Bytes('hello' + '\u4e2d'.repeat(600), 1800) — and verify each chunk's Buffer.byteLength is ≤ 1800 and joining all chunks reproduces the original string.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| disconnect(): void { | ||
| if (this.server) { |
There was a problem hiding this comment.
[Suggestion] disconnect() clears server, seenMessages, seenMessagesCleanup, and serverErrorHandler, but never clears this.accessToken or this.tokenRefresh. After disconnect+reconnect, the new session starts with a potentially stale cached token from the prior session, causing an unnecessary auth failure on the first API call followed by a token refresh round-trip.
| if (this.server) { | |
| this.server.closeAllConnections(); | |
| this.server.close(); | |
| this.server = undefined; | |
| this.accessToken = undefined; | |
| this.tokenRefresh = undefined; |
— qwen3.7-max via Qwen Code /review
0dade15 to
de769d5
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
qqqys
changed the title
|
Rewritten from the previous WeCom self-built application callback approach to the WeCom intelligent robot API mode. The PR branch is now based on current main with a single new implementation commit, using Bot ID + Secret via the official long-connection SDK. The old callback-specific review comments are no longer applicable to the current diff. |
|
Closing this PR per maintainer request. It will be replaced by a fresh PR from the rewritten WeCom intelligent robot branch. |
|
Superseded by #6224. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No Critical issues found. The adapter is well-structured and follows established patterns from dingtalk/feishu/qqbot.
Remaining test coverage gaps (not individually commented): sendMessage empty-text guard (line 128), connect() idempotency guard (line 78), agentId <= 0 validation (line 367), two consecutive invalid-token responses (line 155), and crypto structural validation paths plain.length < 20 / xmlEnd > plain.length (crypto.ts:47-52). Each of these code paths has no corresponding test.
— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
| this.seenMessagesCleanup = setInterval(() => { | ||
| this.removeExpiredSeenMessages(); |
There was a problem hiding this comment.
[Suggestion] seenMessagesCleanup setInterval is never .unref()'d
If the channel is connected but the process should exit (e.g., signal handler skips disconnect()), Node.js stays alive for up to 5 minutes waiting for this timer. The QQ channel adapter already calls .unref() on its reconnect timers for the same reason.
| this.removeExpiredSeenMessages(); | |
| this.seenMessagesCleanup = setInterval(() => { | |
| this.removeExpiredSeenMessages(); | |
| }, DEDUP_TTL_MS); | |
| this.seenMessagesCleanup.unref(); |
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| function resolveEnvValue(value: string): string { | ||
| if (!value.startsWith('$')) return value; |
There was a problem hiding this comment.
[Suggestion] resolveEnvValue has no $ escape mechanism
Any string starting with $ is unconditionally treated as an env-var reference. A WeCom credential that legitimately starts with $ (valid per WeCom spec) cannot be used — users get a confusing "Environment variable X is not set" error.
Consider supporting $$ as a literal $ escape:
| if (!value.startsWith('$')) return value; | |
| function resolveEnvValue(value: string): string { | |
| if (!value.startsWith('$')) return value; | |
| if (value.startsWith('$$')) return value.slice(1); | |
| const envName = value.slice(1); | |
| const resolved = process.env[envName]; | |
| if (!resolved) { | |
| throw new Error(`Environment variable ${envName} is not set.`); | |
| } | |
| return resolved; | |
| } |
— qwen3.7-max via Qwen Code /review
| return Buffer.concat([input, Buffer.alloc(pad, pad)]); | ||
| } | ||
|
|
||
| function encryptPlaintext(xml: string): string { |
There was a problem hiding this comment.
[Suggestion] Test helpers duplicated across 3 test files with divergent implementations
encryptPlaintext(), signature(), and constants (TOKEN, CORP_ID, AES_KEY) are copied into crypto.test.ts, callback.test.ts, and WeComAdapter.test.ts — each with a different padding calculation. The manual-sum variant in callback.test.ts (32 - ((16 + 4 + byteLength(xml) + CORP_ID.length) % 32)) would silently compute a different pad if any component length changes, causing a false test failure that looks like a crypto bug.
Extract the shared helpers and constants into a single test/helpers.ts and import from all three files.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| export function decryptWeComMessage(input: DecryptWeComMessageInput): string { | ||
| const key = decodeEncodingAesKey(input.encodingAesKey); |
There was a problem hiding this comment.
[Nice to have] decodeEncodingAesKey called on every inbound callback
decryptWeComMessage re-validates the 43-char regex and base64-decodes the encodingAesKey on every inbound message, even though the key is static configuration. Deriving the key once in the constructor (or accepting a pre-decoded Buffer parameter) would move validation to startup (failing fast on bad config) and eliminate redundant per-message work.
— qwen3.7-max via Qwen Code /review
|
|
||
| this.server = createServer((req, res) => { | ||
| this.handleRequest(req, res).catch((err) => { | ||
| process.stderr.write( |
There was a problem hiding this comment.
[Suggestion] connect() doesn't coalesce concurrent callers
The guard if (this.server) return checks the Server object assigned synchronously before the await on listen() resolves. A second concurrent connect() caller sees this.server as truthy and returns immediately, but the HTTP server is not yet listening on the port.
Consider storing and returning the in-flight connect promise:
| process.stderr.write( | |
| private connectPromise?: Promise<void>; | |
| async connect(): Promise<void> { | |
| if (this.server) return; | |
| if (this.connectPromise) return this.connectPromise; | |
| this.connectPromise = this.doConnect(); | |
| try { | |
| await this.connectPromise; | |
| } finally { | |
| this.connectPromise = undefined; | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| if (cached && cached.expiresAt > Date.now()) { | ||
| return cached.value; | ||
| } | ||
| if (this.tokenRefresh) { |
There was a problem hiding this comment.
[Suggestion] fetch() follows redirects by default — credentials forwarded to redirect targets
Neither fetchAccessToken() nor sendMessage() sets redirect: 'manual'. Node's fetch() follows up to 20 redirects by default. The corpsecret (in the token URL query string) and access_token (in the send URL query string) would be forwarded to any redirect target.
| if (this.tokenRefresh) { | |
| const resp = await fetchWithContext(url, { | |
| signal: AbortSignal.timeout(WECOM_FETCH_TIMEOUT_MS), | |
| redirect: 'manual', | |
| }, 'WeCom gettoken failed'); | |
| if (resp.status >= 300 && resp.status < 400) { | |
| throw new Error('WeCom API returned a redirect.'); | |
| } |
Apply the same change to sendMessage's fetch call.
— qwen3.7-max via Qwen Code /review
|
|
||
| constructor( | ||
| name: string, | ||
| config: ChannelConfig & Record<string, unknown>, |
There was a problem hiding this comment.
[Suggestion] Constructor signature uses ChannelConfig & Record<string, unknown> unlike other adapters
All other adapters (DingTalk, Feishu, QQ, Telegram, Weixin) declare config: ChannelConfig. WeCom is the only one that widens the public constructor type. This breaks the uniform constructor contract.
| config: ChannelConfig & Record<string, unknown>, | |
| constructor( | |
| name: string, | |
| config: ChannelConfig, | |
| bridge: ChannelAgentBridge, | |
| options?: ChannelBaseOptions, | |
| ) { | |
| super(name, config, bridge, options); | |
| this.wecom = parseWeComConfig(name, config as unknown as Record<string, unknown>); | |
| } |
— qwen3.7-max via Qwen Code /review
| }, DEDUP_TTL_MS); | ||
| process.stderr.write( | ||
| `[WeCom:${this.name}] Listening on ${CALLBACK_HOST}:${this.wecom.webhookPort}.\n`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Several specific code paths lack test coverage
- Empty text guard (
if (!text.trim()) return): no test callssendMessagewith empty/whitespace-only text. connect()idempotency (if (this.server) return): no test callsconnect()a second time on an already-connected channel.- POST with missing query params: a POST callback missing
msg_signature,timestamp, ornoncetriggersrequiredQueryto throw before the decrypt try/catch, returning HTTP 400 — this 400 path is never tested. - Falsy
messageIdbranch in inbound-error handler: all tests providemessageId: 'msg-1', so themessage ? ... : ''ternary and theseenMessages.deleteskip are never exercised.
— qwen3.7-max via Qwen Code /review
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