fix(dingtalk): skip uppercase webhook reaction targets - #5466
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
The regex fix itself is correct — /^https?:\/\//i properly handles case-insensitive URL detection where the old startsWith('http') missed uppercase webhook URLs. The negation is preserved, and the regex is anchored and ReDoS-safe.
Test coverage notes: the new tests cover uppercase HTTPS URLs and conversation IDs on both prompt start/end, but don't cover the most common lowercase https:// case or the HTTP:// (no TLS) branch. Worth adding as follow-up.
|
|
||
| const { DingtalkChannel } = await import('./DingtalkAdapter.js'); | ||
|
|
||
| function createChannel(): DingtalkChannel { |
There was a problem hiding this comment.
[Suggestion] TypeScript errors in test file: DingtalkChannel is a value-import via await import() but used as a type annotation on lines 33 and 52 (TS2749). The config object on line 36 is also not assignable to ChannelConfig (TS2345). These don't block the build (tsconfig excludes src/**/*.test.ts), but they will show as red squiggles in editors.
| function createChannel(): DingtalkChannel { | |
| import type { DingtalkChannel } from './DingtalkAdapter.js'; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| groupPolicy: 'open', | ||
| groups: {}, | ||
| }, | ||
| {} as never, |
There was a problem hiding this comment.
[Suggestion] {} as never is semantically misleading — never is TypeScript's uninhabited bottom type asserting the value can never exist, yet {} is a real runtime object. The rest of the test file uses the as unknown as Target double-cast pattern for escaping private access. Consider using that pattern here for consistency:
| {} as never, | |
| {} as unknown as AcpBridge, |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| "build": "tsc --build" | ||
| "build": "tsc --build", | ||
| "test": "vitest run", | ||
| "test:ci": "vitest run" |
There was a problem hiding this comment.
[Suggestion] "test:ci": "vitest run" is identical to "test" on the line above. No other channel package (qqbot, weixin, feishu, telegram) has a separate test:ci script. Unless a CI pipeline specifically invokes test:ci and expects it to differ from test, this adds no value and creates confusion about which to use.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
✅ Local end-to-end verification (real
|
| Probe | BASE 5924ab3a |
PR 75bfcd13 |
|---|---|---|
onPromptStart('HTTPS://…/robot/send?…') |
❌ POST emotion/reply, openConversationId="HTTPS://…webhook" |
✅ no request (skipped) |
onPromptEnd('HTTPS://…/robot/send?…') |
❌ POST emotion/recall, openConversationId="HTTPS://…webhook" |
✅ no request (skipped) |
onPromptStart('cid-123') — control |
✅ POST emotion/reply, openConversationId="cid-123" |
✅ POST emotion/reply, openConversationId="cid-123" |
onPromptStart('https://…') lowercase — control |
✅ skipped | ✅ skipped |
========== BASE (5924ab3a) ==========
✅ onPromptStart UPPERCASE webhook fetch -> emotion/reply openConversationId="HTTPS://oapi.dingtalk.com/robot/send?access_token=token"
✅ onPromptEnd UPPERCASE webhook fetch -> emotion/recall openConversationId="HTTPS://oapi.dingtalk.com/robot/send?access_token=token"
✅ onPromptStart real conversationId fetch -> emotion/reply openConversationId="cid-123"
✅ onPromptStart lowercase webhook NO fetch (reaction skipped)
========== PR (75bfcd13) ==========
✅ onPromptStart UPPERCASE webhook NO fetch (reaction skipped)
✅ onPromptEnd UPPERCASE webhook NO fetch (reaction skipped)
✅ onPromptStart real conversationId fetch -> emotion/reply openConversationId="cid-123"
✅ onPromptStart lowercase webhook NO fetch (reaction skipped)
So on BASE an uppercase webhook URL is mis-classified as a conversation ID and the bot fires a malformed emotion reply/recall POST with the webhook URL stuffed into openConversationId; on PR it is correctly treated as a webhook fallback and skipped. Real conversation IDs still react on both builds (no regression), and lowercase webhooks were already skipped on both (confirming the bug is specifically case-sensitivity).
Unit test + revert-proof + static checks
vitest runon the PR source → 3/3 pass (DingtalkAdapter.test.ts).- Revert-proof: running the PR's new test file against the base source makes the two uppercase tests fail (
skips uppercase webhook URLs when starting/ending a prompt) while the conversation-ID control passes — they are genuine regression tests, not always-green. - New
test/test:ciscripts run the package suite: 29/29 pass (pre-existingmarkdown.test.ts26 + newDingtalkAdapter.test.ts3), wiring the adapter tests into workspace CI. - ESLint clean (exit 0); Prettier clean on all 3 files;
tsc --buildclean.
Note
The new regex also tightens the non-URL branch: the old startsWith('http') would mis-skip a conversation ID literally beginning with http (e.g. httpfoo), whereas /^https?:\/\//i only matches real http(s):// schemes. This is a small correctness improvement with no downside for real DingTalk conversation IDs.
Verdict: LGTM — the fix resolves the uppercase-webhook reaction bug end-to-end with no regression.
🇨🇳 中文版本（点击展开）
✅ 本地端到端验证（真实 fetch 边界，tmux）
通过驱动真实编译产物中的 DingtalkAdapter（onPromptStart / onPromptEnd → attachReaction/recallReaction → emotionApi），并在 emotion API 实际使用的真实 global.fetch 处拦截来验证——没有 mock 修复本身的任何方法。仅对修复未触及的两个外部依赖（DingTalk stream SDK 和 ChannelBase）做桩，方式与 PR 自带测试一致。两个版本都编译为真实 dist：
- BASE = merge-base
5924ab3a（!chatId.startsWith('http')） - PR = head
75bfcd13（!/^https?:\/\//i.test(chatId)）
发出一个 reaction == 一次 POST https://api.dingtalk.com/v1.0/robot/emotion/{reply|recall}。
A/B 结果（真实 dist + 真实 fetch）
| 探针 | BASE 5924ab3a |
PR 75bfcd13 |
|---|---|---|
onPromptStart('HTTPS://…/robot/send?…') |
❌ POST emotion/reply，openConversationId="HTTPS://…webhook" |
✅ 无请求（跳过） |
onPromptEnd('HTTPS://…/robot/send?…') |
❌ POST emotion/recall，openConversationId="HTTPS://…webhook" |
✅ 无请求（跳过） |
onPromptStart('cid-123') —— 对照 |
✅ POST，openConversationId="cid-123" |
✅ POST，openConversationId="cid-123" |
onPromptStart('https://…') 小写 —— 对照 |
✅ 跳过 | ✅ 跳过 |
也就是说：在 BASE 上，大写的 webhook URL 被误判为会话 ID，机器人会带着被塞进 openConversationId 的 webhook URL 发出错误的 emotion reply/recall 请求；在 PR 上则正确识别为 webhook 兜底并跳过。真实会话 ID 在两个版本都正常加 reaction（无回归），小写 webhook 在两个版本本来就会跳过（说明该 bug 正是大小写敏感导致的）。
单元测试 + 反向验证 + 静态检查
- 在 PR 源码上
vitest run→ 3/3 通过（DingtalkAdapter.test.ts）。 - 反向验证：把 PR 新增的测试文件放到 base 源码上运行，两个大写用例失败（
skips uppercase webhook URLs when starting/ending a prompt），而会话 ID 对照用例通过——证明它们是真正的回归测试，而非恒绿。 - 新增的
test/test:ci脚本会运行整个包的测试套件：29/29 通过（既有的markdown.test.ts26 个 + 新增DingtalkAdapter.test.ts3 个），将适配器测试接入 workspace CI。 - ESLint 通过（exit 0）；Prettier 三个文件均通过；
tsc --build通过。
补充说明
新正则也顺带收紧了非 URL 分支：旧的 startsWith('http') 会把字面以 http 开头的会话 ID（例如 httpfoo）误跳过，而 /^https?:\/\//i 只匹配真正的 http(s):// 协议。这是一处小的正确性改进，对真实 DingTalk 会话 ID 没有任何副作用。
结论：LGTM —— 修复在端到端层面解决了大写 webhook 的 reaction 误触发问题，且无回归。
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @tt-a1i, thanks for the fix!
The PR body doesn't follow our PR template. Could you update it to match? Specifically:
- Missing sections:
## What this PR does,## Why it's needed,## Risk & Scope,## Linked Issues, and the## Reviewer Test Plansubsections (### How to verify,### Evidence (Before & After),### Tested on) - Current headings (
## Summary,## Test Plan) don't map to the template structure - Missing Chinese translation in a
<details>block
The actual content looks reasonable — it's mostly a restructuring exercise. Once the template is filled in, happy to re-run triage.
中文说明
嗨 @tt-a1i，感谢修复！
PR 正文没有按照我们的 PR 模板 填写。能否更新一下使其匹配？具体来说：
- 缺少的章节：
## What this PR does、## Why it's needed、## Risk & Scope、## Linked Issues，以及## Reviewer Test Plan的子章节（### How to verify、### Evidence (Before & After)、### Tested on） - 当前的标题（
## Summary、## Test Plan）与模板结构不匹配 - 缺少
<details>中的中文翻译
内容本身看起来合理——主要是重新按模板组织一下。模板填好后可以重新跑 triage。
— Qwen Code · qwen3.7-max
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— DeepSeek/deepseek-v4-pro via Qwen Code /review
✅ Maintainer verification — code is correct & verifiedBuilt and ran the real test suite locally (under tmux) against the PR head Environment
Results — full PR test plan reproduced
(29 = 3 new A/B proof that the tests actually guard the bugTo confirm the new tests are not no-ops, I reverted only Restoring the fix → back to green ( Root cause & fix (confirmed in source)
Code assessment
Note on the
|
| 步骤 | 命令 | 结果 |
|---|---|---|
| 单元测试 | npm --workspace packages/channels/dingtalk run test:ci |
✅ 29 通过 / 0 失败（2 个文件） |
| 构建 | npm --workspace packages/channels/dingtalk run build |
✅ 退出码 0 |
| Prettier | prettier --check package.json DingtalkAdapter.{ts,test.ts} |
✅ 退出码 0 |
| ESLint | eslint DingtalkAdapter.ts DingtalkAdapter.test.ts |
✅ 退出码 0 |
| 空白检查 | git diff --check |
✅ 退出码 0 |
（29 = 3 个新增 DingtalkAdapter.test.ts 用例 + 26 个既有 markdown.test.ts 用例，见下方说明。）
A/B 验证（证明测试确实能拦住该 bug）
为确认新测试不是空跑，我仅把 isConversationId 改回原来的 !chatId.startsWith('http') 后重跑：两个「大写 webhook URL」用例失败（expected "spy" to not be called at all, but actually been called 1 times），共 2 失败 / 27 通过；恢复修复后重新变绿（29 通过）。即两个大写用例没有修复时失败、有修复时通过，而 conversation-ID 用例（及 26 个 markdown 用例）两种情况都通过——测试精准隔离了被修复的行为。
根因与修复（已在源码确认）
onPromptStart / onPromptEnd 通过 isConversationId(chatId) 来决定是否打 reaction，其中 chatId = conversationId || sessionWebhook。旧的大小写敏感 startsWith('http') 会把大写 webhook URL（HTTPS://oapi.dingtalk.com/robot/send?...）误判为 conversation ID，于是对一个 webhook 目标错误地调用了 attachReaction / recallReaction，而非跳过。新的 /^https?:\/\//i 大小写不敏感、有锚定、且 ReDoS 安全；取反逻辑保持不变。
代码评估
- 单行最小修复，符合仓库中
^https?://大小写不敏感的惯例。 - 额外收益： 新增的
test/test:ci脚本不只跑新文件，还把既有的markdown.test.ts（26 个用例）纳入了 CI——此前该包没有 test 脚本来运行它们。 - 测试脚本接线（根部 hoisted 的
vitest，本地不声明）与兄弟包channels/qqbot/channels/telegram一致；CI 上三大 OS 的 Test 任务均为绿。
关于 CHANGES_REQUESTED 状态
唯一一条「requested changes」来自 qwen-code-ci-bot，且仅针对 PR 模板的小标题——并未要求任何代码改动。其余 CI 全绿（Classify、CodeQL、Lint、ubuntu/macos/windows · Node 22.x 测试）。从正确性角度可以合并——仅剩 PR 描述格式待定，由你决定。
|
updated the PR description to match the template. thanks. |
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ On direction: this is a clean correctness fix for a real bug. The DingTalk adapter's On approach: the one-line fix ( Scope is minimal: one regex fix, three focused tests, two script additions. No drive-by changes. Moving on to code review. 🔍 中文说明感谢 @tt-a1i 的 PR！ 模板完整 ✓ 方向：这是一个针对真实 bug 的正确性修复。DingTalk adapter 的 方案：单行修复（ 范围最小化：一个正则修复、三个针对性测试、两个脚本新增，没有夹带无关改动。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: Before reading the diff, I'd replace Diff assessment: The PR matches the independent proposal. Three files, all necessary:
No correctness bugs, no security concerns, no AGENTS.md violations. The regex pattern is ReDoS-safe (no nested quantifiers or alternation blowup). Test ResultsAll checks pass locally on the PR head ( Revert-proof verification — reverting only The two uppercase-webhook tests fail without the fix and pass with it. The conversation-ID control passes either way. These are genuine regression guards. CI (all green): Classify ✓ · CodeQL ✓ · Lint ✓ · Test (ubuntu/macos/windows · Node 22.x) ✓ Note on tmux testing: This fix targets an internal adapter path (DingTalk webhook → emotion API routing), not user-facing CLI behavior. It cannot be meaningfully driven through 中文说明代码审查独立方案： 在读 diff 之前，我会把 Diff 评估： PR 与独立方案吻合。三个文件均必要：
无正确性 bug、无安全隐患、无 AGENTS.md 违规。正则模式 ReDoS 安全（无嵌套量词或交替膨胀）。 测试结果本地 PR head（ 反向验证——仅将 CI（全绿）：Classify ✓ · CodeQL ✓ · Lint ✓ · Test (ubuntu/macos/windows · Node 22.x) ✓ 关于 tmux 测试： 此修复针对内部 adapter 路径（DingTalk webhook → emotion API 路由），非用户可见的 CLI 行为，无法通过终端 — Qwen Code · qwen3.7-max |
ReflectionThis is what a good bug-fix PR looks like. One-line fix, three targeted tests, and the tests actually guard the bug (confirmed by reverting the fix — both uppercase cases fail immediately). The approach matches my independent proposal exactly. The regex CI is fully green across all platforms and all check types. The Approved. ✅ 中文说明反思这是一个优秀的 bug 修复 PR 应有的样子。单行修复、三个针对性测试，且测试确实能拦住该 bug（回退修复后两个大写用例立即失败）。 方案与我的独立提案完全一致。正则 CI 在所有平台和所有检查类型上全绿。 已批准 ✅ — 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. ✅
3 participants
What this PR does
HTTP://andHTTPS://DingTalk webhook URLs as webhook fallback targets instead of conversation IDs.testandtest:ciscripts so the adapter tests are included in the package test workflow.Why it's needed
The DingTalk adapter skips prompt reactions for webhook fallback targets, but the old
startsWith('http')check only handled lowercase schemes. An uppercase webhook URL could be misclassified as a conversation ID and then used asopenConversationIdin DingTalk emotion API calls.Reviewer Test Plan
How to verify
onPromptStartandonPromptEndskip uppercase webhook URLs such asHTTPS://oapi.dingtalk.com/robot/send?....npm --workspace packages/channels/dingtalk run test.npm --workspace packages/channels/dingtalk run test:ci.npm --workspace packages/channels/dingtalk run build.npx prettier --check packages/channels/dingtalk/package.json packages/channels/dingtalk/src/DingtalkAdapter.ts packages/channels/dingtalk/src/DingtalkAdapter.test.ts.npx eslint packages/channels/dingtalk/src/DingtalkAdapter.ts packages/channels/dingtalk/src/DingtalkAdapter.test.ts.Evidence (Before & After)
Before:
onPromptStart('HTTPS://...')andonPromptEnd('HTTPS://...')could send DingTalk emotionreply/recallrequests with the webhook URL incorrectly placed inopenConversationId. After: uppercase webhook URLs are detected as webhook targets and skipped; real conversation IDs still send reactions. The new tests are revert-proof because the uppercase webhook cases fail against the old lowercase-only check.Tested on
Environment (optional)
Local DingTalk package test/build/lint/prettier checks on macOS.
Risk & Scope
http(s)://webhook URLs, so a literal conversation ID beginning withhttpbut not a URL would no longer be skipped.Linked Issues
Fixes #5465
中文说明
What this PR does
HTTP://和HTTPS://DingTalk webhook URL 识别为 webhook fallback 目标，而不是会话 ID。test和test:ci脚本，让 adapter 测试进入 package 测试流程。Why it's needed
DingTalk adapter 会跳过 webhook fallback 目标的 prompt reaction，但旧的
startsWith('http')只覆盖小写协议。大写 webhook URL 可能被误判成会话 ID，然后作为openConversationId传给 DingTalk emotion API。Reviewer Test Plan
How to verify
onPromptStart和onPromptEnd会跳过HTTPS://oapi.dingtalk.com/robot/send?...这类大写 webhook URL。npm --workspace packages/channels/dingtalk run test。npm --workspace packages/channels/dingtalk run test:ci。npm --workspace packages/channels/dingtalk run build。Evidence (Before & After)
修复前：
onPromptStart('HTTPS://...')和onPromptEnd('HTTPS://...')可能发出 DingTalk emotionreply/recall请求，并把 webhook URL 错误放进openConversationId。修复后：大写 webhook URL 会被识别为 webhook 目标并跳过；真实 conversation ID 仍然会发送 reaction。新增测试是 revert-proof 的，因为大写 webhook 用例在旧的小写限定检查上会失败。Tested on
Environment (optional)
在 macOS 上运行了 DingTalk package 的 test/build/lint/prettier 检查。
Risk & Scope
http(s)://webhook URL，所以字面以http开头但不是 URL 的 conversation ID 不再被跳过。Linked Issues
Fixes #5465
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.