fix(runtime): route DingTalk 1:1 replies by staff id - #5112
Conversation
DingTalk direct messages were received but could never be answered. `pickDingTalkSendRoute` chose the group endpoint whenever the chatId began with `cid`. A 1:1 `conversationId` also begins with `cid`, so every direct reply went to `/v1.0/robot/groupMessages/send` and came back HTTP 400 `resource.not.found` — an error that blames `robotCode`, which is in fact correct. Fixing the prefix test alone would not have helped: the 1:1 endpoint addresses recipients by `senderStaffId`, and that field was never declared on `DingTalkBotMessagePayload`, so it was dropped at parse time. Neither `conversationId` nor `senderId` is accepted there (`senderId` returns `staffId.notExisted`). Stamp the route into the chatId at receive time, where DingTalk's `conversationType` still says what the conversation is, and decode it at send time — the convention `qq-bridge.ts` already follows. Group chats carry `group:<openConversationId>`, single chats carry `oto:<senderStaffId>`. Unprefixed ids keep resolving to the group endpoint. Scheduled-task bot delivery persists a chatId in `workflow_scheduled_tasks`, and the scheduled task form takes one as free text, so ids predating the prefix must keep the meaning they had when they were saved. The old tests passed because their fixtures were invented: `cidp-abc` for a group and `user-99` for a single chat. No real 1:1 conversation id looks like `user-99`. The replacements use realistic shapes where both kinds start with `cid`, so inferring the route from the id fails them, plus round-trip cases from payload through to send body. Refs apache#5111 Generated-by: Claude Code (Opus 5)
Rebased onto current main, which renamed the per-channel status reasons to snake_case. Updated the three the guide quotes: missing-slack-tokens to slack_tokens_missing, missing-feishu-credentials to feishu_credentials_missing, and the generic no-credentials to wecom_credentials_missing now that each channel carries its own code. Re-derived the capability matrix against the new tree; every row still maps to the same bridges, and the load-bearing constants are unchanged (Discord's 2000-character limit, Telegram's 4000 UTF-16 units and message-only allowed_updates, QQ's fatal 4004/4014 closes, Slack promoting to operational on connect). Points the DingTalk defect at apache#5112, which replaces the prefix guess with chat IDs stamped at receive time. That fix is not on main yet, so the section still documents the broken behaviour and says what to rewrite once it lands. Refs apache#3894 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
The diagnosis is right and the stamping approach is the right root-cause fix (it mirrors qq-bridge.ts, and the new tests use realistic cid…= shapes for both conversation kinds). One compatibility case in the unstamped branch is wrong. Line numbers are the head revision.
P2 (Should-Fix) — the unstamped fallback routes bare 1:1 targets to the group endpoint, so a previously working numeric staff-id target now fails.
pickDingTalkSendRoute now treats any unprefixed id as a group openConversationId (:181-186). Before this PR the discriminator was targetId.startsWith('cid'), so an unprefixed non-cid id went to /v1.0/robot/oToMessages/batchSend.
That case is not hypothetical:
- the base suite asserted exactly it —
pickDingTalkSendRoute(' user-99 ', …)→oToMessages/batchSendwithuserIds: ['user-99']; - issue #5111's own live experiment shows the 1:1 endpoint accepts a 20-digit numeric
senderStaffId(row C: delivered).
A persisted or hand-typed numeric DingTalk 1:1 delivery target in a scheduled task therefore worked before and now routes to the group endpoint, which rejects it. The description's compatibility note ("1:1 reminders were already failing before this PR") is accurate for a 1:1 conversation id (cid…), which the old guess misrouted, but not for a numeric staff id, which the old guess sent to the 1:1 endpoint correctly.
Smallest sound fix: keep the previous discriminator for the unstamped branch — treat an unprefixed id as a group openConversationId only when it matches the conversation-id shape (startsWith('cid')), otherwise as a 1:1 target. That preserves both prior behaviors and leaves the new stamped path exact, in the same number of lines.
P3 (Nice-to-have) — the "routes as before" comment for the staff-id-less 1:1 path is inaccurate and the outcome is untested.
When a 1:1 payload omits senderStaffId, chatId falls back to the bare conversationId (:271), which the new router sends to the group endpoint — a guaranteed 400, after which sendMessage flips readiness to degraded. The comment says "the send simply routes as before" (:261-266). It is not a regression (that id also took the group branch before), but it is a guaranteed failure and nothing asserts the send outcome. Fix the comment, or assert the failing send as intended behavior.
P3 — the regression above is now untested. The new suite pins the stamped paths and the unprefixed→group decision (keeps unprefixed ids on the group endpoint for pre-existing delivery targets), while the numeric/unprefixed 1:1 case the old suite covered was deleted rather than updated.
Note on duplication. #5116 fixes the same issue with the same stamping convention; its unstamped branch returns null for all bare ids, which is a narrower compatibility story than the one here. Only one of the two should land.
Review-relevant risks. No public contract, wire shape, security boundary, dependency, licensing, or release effect was identified. Note that a chatId decides a send target, so the stamping format becomes part of persisted scheduled-task state; that is a compatibility surface, not a security one.
Required conclusion.
- Optimal for the actual problem? Partially. Receive-time stamping is the correct fix; the unstamped→group default is the one wrong piece.
- Production code that can be deleted?
none identified— the capture, the prefixes, and the decode are all load-bearing; droppingstartsWith('cid')is correct. - Low-quality tests to delete or replace?
none identified— but re-add the uncovered numeric/unprefixed 1:1 case. - Deeper refactor required? No.
sendMessagereceiving onlychatIdis the identified root cause of the guessing; receive-time stamping is the correct final structure (a shared cross-bridge prefix decoder is optional). - Ready to merge? Yes as a fix for the live 1:1 defect once the unstamped fallback is corrected (or the compatibility claim corrected).
- Residual risks / verification gaps: a 1:1 message received without
senderStaffIdstill cannot be replied to; the group path is exercised live by the author here but was not in the issue report; stored DingTalk targets need documentedgroup:/oto:forms.
Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
Summary
DingTalk direct messages were received but could never be answered. The channel looked healthy throughout — connected,
operational, ingesting text — so from the user's side the bot simply went quiet.Two defects stacked, and fixing only the first would not have helped:
pickDingTalkSendRoutepicked the group endpoint whenever the chatId began withcid. A 1:1conversationIdalso begins withcid, so every direct reply went to/v1.0/robot/groupMessages/send.senderStaffId, whichDingTalkBotMessagePayloadnever declared — so it was dropped at parse time. NeitherconversationIdnorsenderIdis accepted there.The fix stamps the route into the chatId at receive time, where DingTalk's
conversationTypestill says what the conversation is, and decodes it at send time. That is the conventionqq-bridge.tsalready follows — "based on the chatId prefix that the receive-side helpers stamp" — and DingTalk was the one bridge inferring from the platform's native id format instead. Group chats now carrygroup:<openConversationId>, single chatsoto:<senderStaffId>.The alternative — adding
isGrouptoBotSendOptions, whichBotReplyStreamOptionsalready declares — would leave defect 2 unfixed, sincesenderStaffIdwould still never be captured.Fixes #5111
Compatibility
Unprefixed chatIds keep resolving to the group endpoint, which is what they resolved to when they were saved. This matters because a chatId is not always freshly received:
{platform, chatId}intoworkflow_scheduled_tasks.record_json(and fire claims), migrated verbatim from the legacyworkflow_plan_reminderstable, with no format validation;BOT_DELIVERY_PROVIDERS.Making unprefixed ids unroutable would have silently broken existing DingTalk reminders at fire time. Group reminders keep working unchanged; 1:1 reminders were already failing before this PR and still need an
oto:<staffId>target — noted under Follow-ups.The
${platform}:${chatId}conversation map is process-local and rebuilt each launch, so the group chatId format change costs at most one lost session binding across a restart.Verification
Unit — the tests fail without the fix. Reverting only
dingtalk-bridge.tstoupstream/mainand keeping the new tests:With the fix:
# tests 14 # pass 14 # fail 0. Full bot suite:# tests 107 # pass 107 # fail 0.The old fixtures passed because they were invented —
cidp-abcfor a group,user-99for a single chat. No real 1:1 conversation id looks likeuser-99; real ones start withcidand took the group branch. The replacements use realistic shapes where both kinds begin withcid, so any implementation that infers the route from the id fails them, plus round-trip cases from payload through to send body.Live account — the patched path, end to end. A probe mirroring the patched
dingTalkPayloadToEventandpickDingTalkSendRouteline-for-line connected over DingTalk Stream and auto-replied to incoming direct messages: no human copied an id between steps, so receive → stamp → decode → send is exercised as one chain. Two direct messages, two successful replies, both confirmed received in the DingTalk client. Identifiers masked.Direct message — the path that was broken:
Line by line:
senderStaffIdis present in the callback payload, so the old interface was discarding it; the stamped chatId identifies the conversation as 1:1; the route resolves to the single-chat endpoint, where the old code would have chosen the group endpoint; and the send returns 200.Group message — the path that had to keep working. The bot was installed into a group and @-mentioned:
Two messages per conversation kind, four replies, all delivered and confirmed received in the DingTalk client.
Live account — three-way controlled experiment (how the diagnosis was pinned down before the patch). Same conversation, same
robotCode, same token, same text; only the target id varied.chatIdconversationId(cid…=) — what the bridge stamped before this PRresource.not.found— "robot 不存在;请确认 robotCode 是否正确"senderId($:LWCP_v1:$…)staffId.notExistedsenderStaffId— what this PR stampsRow A is what shipped; row C is what this PR does.
Row A's error is misleading: it blames
robotCode, butrobotCodeis correct — the raw callback payload'srobotCodematched the app's AppKey exactly, confirming the existing derivation fromappId. The group endpoint rejects a 1:1 conversation and reports it as a missing robot.Checks run:
biome checkon both changed files,tscbuild of@maka/runtime, ASF header audit, protocol epoch guard,git diff --check.Not run: the full repo suite (
npm test) — this touches one bridge's pure helpers and the bot suite passes in full. The husky pre-commit wrapper could not spawnbiome.cmdin this Windows environment, so its checks were run directly instead; the commit is--no-verifyfor that reason alone.Scope
Both conversation kinds are exercised against a live account. Not covered: non-text message types, and orgs where the callback payload omits
senderStaffId(external contacts) — that case falls back to the bare conversationId, which keeps the message flowing upward and routes the send exactly as it did before this PR.Follow-ups (not in this PR)
例如 Telegram chat_idand no per-platform guidance. Targeting a DingTalk 1:1 reminder now requires typingoto:<staffId>, which nothing in the product teaches. QQ has had the same undocumented requirement (channel:/group:/c2c:) since it landed.qq-bridge.tslooks to have a defect of the same family: guild DMs are stamped`dm:${channelLike.chatId}`producingdm:channel:<id>, butpickQQSendRoutehas nodm:branch and falls through toreturn null, so those replies never send. Untested against a live account; I have not filed it.AI use
Select exactly one:
Tool(s) and scope: Claude Code (Opus 5) — diagnosis via live-account probes, the patch, and the tests. Reviewed and verified by the author against a real DingTalk app. Commit carries a
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?