From 4bf30d9552eff6afb4cbea4c58ce7fc09d77289f Mon Sep 17 00:00:00 2001 From: ying-hua <60057611+ying-hua@users.noreply.github.com> Date: Wed, 9 Sep 2026 22:03:35 +0800 Subject: [PATCH 1/2] fix(runtime): route DingTalk 1:1 replies by staff id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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:`, single chats carry `oto:`. 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 #5111 Generated-by: Claude Code (Opus 5) --- .../bots/__tests__/dingtalk-bridge.test.ts | 129 +++++++++++++++--- packages/runtime/src/bots/dingtalk-bridge.ts | 86 +++++++++--- 2 files changed, 177 insertions(+), 38 deletions(-) diff --git a/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts b/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts index 5f2779e1d6..a80115c5dc 100644 --- a/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts +++ b/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts @@ -37,27 +37,55 @@ describe('decideDingTalkClose (PR-BOT-DINGTALK-OPERATIONAL-0)', () => { }); }); +// Realistic identifier shapes. Both conversation kinds begin with `cid`, +// which is the whole point: any implementation that infers the route from +// DingTalk's own id format instead of the stamped prefix fails these. +const SINGLE_CONVERSATION_ID = 'cidEXAMPLEsingle0Ab1Cd2Ef3Gh4Ij5Kl6Mn7Op8='; +const GROUP_CONVERSATION_ID = 'cidEXAMPLEgroup9Zy8Xw7Vu6Ts5Rq4Po3Nm2Lk1J='; +const SENDER_ID = '$:LWCP_v1:$0Ab1Cd2Ef3Gh4Ij5Kl6Mn7Op8Qr9St0Uv'; +const SENDER_STAFF_ID = '01234567890123456789'; + describe('pickDingTalkSendRoute', () => { - it('routes group and direct targets while rejecting empty ids', () => { - assert.deepEqual(pickDingTalkSendRoute(' cidp-abc ', 'app-key-1', 'hello'), { - path: '/v1.0/robot/groupMessages/send', - body: { - robotCode: 'app-key-1', - openConversationId: 'cidp-abc', - msgKey: 'sampleText', - msgParam: '{"content":"hello"}', - }, - }); - assert.deepEqual(pickDingTalkSendRoute(' user-99 ', 'app-key-1', 'hi'), { + it('routes a stamped single chat to the 1:1 endpoint by staff id', () => { + assert.deepEqual(pickDingTalkSendRoute(` oto:${SENDER_STAFF_ID} `, 'app-key-1', 'hi'), { path: '/v1.0/robot/oToMessages/batchSend', body: { robotCode: 'app-key-1', - userIds: ['user-99'], + userIds: [SENDER_STAFF_ID], msgKey: 'sampleText', msgParam: '{"content":"hi"}', }, }); + }); + + it('routes a stamped group chat to the group endpoint', () => { + assert.deepEqual( + pickDingTalkSendRoute(` group:${GROUP_CONVERSATION_ID} `, 'app-key-1', 'hello'), + { + path: '/v1.0/robot/groupMessages/send', + body: { + robotCode: 'app-key-1', + openConversationId: GROUP_CONVERSATION_ID, + msgKey: 'sampleText', + msgParam: '{"content":"hello"}', + }, + }, + ); + }); + + it('keeps unprefixed ids on the group endpoint for pre-existing delivery targets', () => { + // Scheduled-task delivery targets persisted before the prefix existed, + // and ids typed by hand into the scheduled task form. + const route = pickDingTalkSendRoute(GROUP_CONVERSATION_ID, 'app-key-1', 'hello'); + assert.equal(route?.path, '/v1.0/robot/groupMessages/send'); + assert.equal(route?.body.openConversationId, GROUP_CONVERSATION_ID); + }); + + it('rejects empty and prefix-only ids', () => { assert.equal(pickDingTalkSendRoute(' ', 'app-key-1', 'hi'), null); + assert.equal(pickDingTalkSendRoute('oto:', 'app-key-1', 'hi'), null); + assert.equal(pickDingTalkSendRoute('oto: ', 'app-key-1', 'hi'), null); + assert.equal(pickDingTalkSendRoute('group:', 'app-key-1', 'hi'), null); }); }); @@ -93,12 +121,13 @@ describe('classifyDingTalkSendResponse', () => { }); describe('dingTalkPayloadToEvent', () => { - it('maps direct and group messages with stable identity fallbacks', () => { + it('addresses a single chat by staff id, not by conversation id', () => { const event = dingTalkPayloadToEvent( { - senderId: 'user-1', + senderId: SENDER_ID, + senderStaffId: SENDER_STAFF_ID, senderNick: 'Alice', - conversationId: 'cidp-single', + conversationId: SINGLE_CONVERSATION_ID, conversationType: '1', text: { content: 'hello' }, robotCode: 'app-key-1', @@ -107,23 +136,81 @@ describe('dingTalkPayloadToEvent', () => { ); assert.ok(event); assert.equal(event!.platform, 'dingtalk'); - assert.equal(event!.userId, 'user-1'); + assert.equal(event!.userId, SENDER_ID); assert.equal(event!.userName, 'Alice'); - assert.equal(event!.chatId, 'cidp-single'); + assert.equal(event!.chatId, `oto:${SENDER_STAFF_ID}`); assert.equal(event!.isGroup, false); assert.equal(event!.text, 'hello'); - assert.equal(event!.sourceMessageId, 'cidp-single:1700000000000'); + assert.equal(event!.sourceMessageId, `${SINGLE_CONVERSATION_ID}:1700000000000`); + }); + + it('addresses a group chat by conversation id', () => { const groupEvent = dingTalkPayloadToEvent( { - senderId: 'user-2', - conversationId: 'cidp-group', + senderId: SENDER_ID, + senderStaffId: SENDER_STAFF_ID, + conversationId: GROUP_CONVERSATION_ID, conversationType: '2', text: { content: 'hi' }, }, 1, ); + assert.ok(groupEvent); assert.equal(groupEvent!.isGroup, true); - assert.equal(groupEvent!.userName, 'user-2'); + assert.equal(groupEvent!.chatId, `group:${GROUP_CONVERSATION_ID}`); + assert.equal(groupEvent!.userName, SENDER_ID); + }); + + it('round-trips a single chat from receive to the 1:1 send body', () => { + // The regression this guards: a 1:1 conversationId also starts with + // `cid`, so routing on the raw id sends direct replies to the group + // endpoint and DingTalk answers 400 resource.not.found. + const event = dingTalkPayloadToEvent( + { + senderId: SENDER_ID, + senderStaffId: SENDER_STAFF_ID, + conversationId: SINGLE_CONVERSATION_ID, + conversationType: '1', + text: { content: 'ping' }, + }, + 1, + ); + const route = pickDingTalkSendRoute(event!.chatId, 'app-key-1', 'pong'); + assert.equal(route?.path, '/v1.0/robot/oToMessages/batchSend'); + assert.deepEqual(route?.body.userIds, [SENDER_STAFF_ID]); + }); + + it('round-trips a group chat from receive to the group send body', () => { + const event = dingTalkPayloadToEvent( + { + senderId: SENDER_ID, + senderStaffId: SENDER_STAFF_ID, + conversationId: GROUP_CONVERSATION_ID, + conversationType: '2', + text: { content: 'ping' }, + }, + 1, + ); + const route = pickDingTalkSendRoute(event!.chatId, 'app-key-1', 'pong'); + assert.equal(route?.path, '/v1.0/robot/groupMessages/send'); + assert.equal(route?.body.openConversationId, GROUP_CONVERSATION_ID); + }); + + it('still delivers a single chat that carries no staff id', () => { + // Nothing to address a 1:1 reply to, but receiving must not depend on + // being able to reply. + const event = dingTalkPayloadToEvent( + { + senderId: SENDER_ID, + conversationId: SINGLE_CONVERSATION_ID, + conversationType: '1', + text: { content: 'hello' }, + }, + 1, + ); + assert.ok(event); + assert.equal(event!.isGroup, false); + assert.equal(event!.chatId, SINGLE_CONVERSATION_ID); }); it('drops payloads missing text or routing identity', () => { diff --git a/packages/runtime/src/bots/dingtalk-bridge.ts b/packages/runtime/src/bots/dingtalk-bridge.ts index ccd531308d..cd1bda8d1d 100644 --- a/packages/runtime/src/bots/dingtalk-bridge.ts +++ b/packages/runtime/src/bots/dingtalk-bridge.ts @@ -56,6 +56,23 @@ const SEND_RETRY_DELAY_MAX_MS = 30_000; const DINGTALK_TOPIC_BOT_MESSAGES = '/v1.0/im/bot/messages/get'; +const DINGTALK_GROUP_SEND_PATH = '/v1.0/robot/groupMessages/send'; +const DINGTALK_SINGLE_SEND_PATH = '/v1.0/robot/oToMessages/batchSend'; + +/** + * Route markers stamped onto `chatId` when a message is received, so the + * send side can pick an endpoint without re-deriving the conversation + * kind. Same approach as `qq-bridge.ts`, and for the same reason: the + * conversation kind is known exactly at receive time (DingTalk sends + * `conversationType`) and cannot be recovered later from the id alone. + * + * Guessing from DingTalk's own id shapes does not work — 1:1 and group + * `conversationId` values both begin with `cid`, so a prefix test sends + * every direct reply to the group endpoint. + */ +const DINGTALK_GROUP_CHAT_PREFIX = 'group:'; +const DINGTALK_SINGLE_CHAT_PREFIX = 'oto:'; + interface DingTalkConnectionOpenResponse { endpoint: string; ticket: string; @@ -70,6 +87,12 @@ interface DingTalkStreamFrame { interface DingTalkBotMessagePayload { senderId?: string; + /** + * The org-scoped staff id of the sender. This is the only identifier + * `/v1.0/robot/oToMessages/batchSend` accepts in `userIds`: `senderId` + * (a `$:LWCP_v1:$…` handle) is rejected with `staffId.notExisted`. + */ + senderStaffId?: string; senderNick?: string; conversationId?: string; conversationType?: '1' | '2'; // 1 = single chat, 2 = group @@ -127,6 +150,16 @@ export function buildDingTalkSingleSendBody( }; } +/** + * Pure helper: route a send to the right DingTalk REST endpoint based on + * the chatId prefix that `dingTalkPayloadToEvent` stamps. + * + * An unprefixed id is treated as a group `openConversationId`. Those are + * chatIds recorded before this bridge stamped a prefix — persisted + * scheduled-task delivery targets and hand-typed ids in the scheduled + * task form — and group delivery is what they resolved to at the time, + * so keeping that mapping preserves their behavior. + */ export function pickDingTalkSendRoute( chatId: string, robotCode: string, @@ -137,12 +170,21 @@ export function pickDingTalkSendRoute( } | null { const targetId = chatId.trim(); if (!targetId) return null; - const isGroup = targetId.startsWith('cid'); + if (targetId.startsWith(DINGTALK_SINGLE_CHAT_PREFIX)) { + const staffId = targetId.slice(DINGTALK_SINGLE_CHAT_PREFIX.length).trim(); + if (!staffId) return null; + return { + path: DINGTALK_SINGLE_SEND_PATH, + body: buildDingTalkSingleSendBody(staffId, robotCode, text), + }; + } + const openConversationId = targetId.startsWith(DINGTALK_GROUP_CHAT_PREFIX) + ? targetId.slice(DINGTALK_GROUP_CHAT_PREFIX.length).trim() + : targetId; + if (!openConversationId) return null; return { - path: isGroup ? '/v1.0/robot/groupMessages/send' : '/v1.0/robot/oToMessages/batchSend', - body: isGroup - ? buildDingTalkGroupSendBody(targetId, robotCode, text) - : buildDingTalkSingleSendBody(targetId, robotCode, text), + path: DINGTALK_GROUP_SEND_PATH, + body: buildDingTalkGroupSendBody(openConversationId, robotCode, text), }; } @@ -210,21 +252,34 @@ export function dingTalkPayloadToEvent( if (!payload || typeof payload !== 'object') return null; const content = payload.text?.content; if (typeof content !== 'string' || content.length === 0) return null; - const chatId = payload.conversationId; + const conversationId = payload.conversationId; const userId = payload.senderId; - if (typeof chatId !== 'string' || chatId.length === 0) return null; + if (typeof conversationId !== 'string' || conversationId.length === 0) return null; if (typeof userId !== 'string' || userId.length === 0) return null; + const isGroup = payload.conversationType === '2'; + const staffId = typeof payload.senderStaffId === 'string' ? payload.senderStaffId.trim() : ''; + // Stamp the route while the conversation kind is still known. A 1:1 + // reply must address the sender's staff id, not the conversation, so + // that is what the chatId carries. When the payload omits + // `senderStaffId` there is no address to reply to, so fall back to the + // bare conversationId: the message is still delivered upward and keeps + // a stable per-conversation key, and the send simply routes as before. + const chatId = isGroup + ? `${DINGTALK_GROUP_CHAT_PREFIX}${conversationId}` + : staffId + ? `${DINGTALK_SINGLE_CHAT_PREFIX}${staffId}` + : conversationId; return { platform: 'dingtalk', userId, userName: payload.senderNick ?? userId, chatId, - isGroup: payload.conversationType === '2', + isGroup, text: content, // DingTalk Stream callbacks do not carry the original message id; // use a synthetic key so downstream contracts that key off // `sourceMessageId` still get a unique value. - sourceMessageId: `${chatId}:${receivedAt}`, + sourceMessageId: `${conversationId}:${receivedAt}`, receivedAt, }; } @@ -374,14 +429,11 @@ export class DingTalkBotBridge extends WsBridgeBase implements SendCapable { } /** - * DingTalk REST send. We treat any chatId with a `cidp` prefix as a - * group conversation; pure-numeric or other prefixes route to the - * single-user batch API. The caller (main.ts) already knows whether - * the bot conversation is a group via the BotMessageEvent.isGroup - * flag, but the bridge's `sendMessage` only sees the chatId — so we - * make a conservative split based on the `conversationType` hint - * baked into the chatId structure: group conversation IDs start with - * `cid` per DingTalk's open platform docs. + * DingTalk REST send. `sendMessage` only sees the chatId, so the + * conversation kind travels inside it: `dingTalkPayloadToEvent` + * stamps `group:` or `oto:` at receive time, where DingTalk's + * `conversationType` is still available, and + * `pickDingTalkSendRoute` decodes it here. */ async sendMessage( chatId: string, From ed622ec9bd299d337aa0d3c41aaa5363bbdd8150 Mon Sep 17 00:00:00 2001 From: ying-hua <60057611+ying-hua@users.noreply.github.com> Date: Fri, 11 Sep 2026 22:12:54 +0800 Subject: [PATCH 2/2] fix(runtime): keep unstamped DingTalk staff ids on the 1:1 endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of this branch routed every unstamped chatId to the group endpoint, on the reasoning that an unstamped id should keep resolving to what it resolved to before. That reasoning was wrong: what an unstamped id resolved to depended on its shape. `startsWith('cid')` sent `cid…` conversation ids to the group endpoint and everything else to the 1:1 endpoint, and the 1:1 endpoint accepts a bare staff id — so a persisted or hand-typed numeric delivery target was working, and collapsing both shapes onto the group endpoint broke it. Keep the pre-stamping discriminator for unstamped ids so both of its outcomes survive. The old guess was only ever wrong for a 1:1 conversation id, which the stamped path now handles. Also correct the comment on the staff-id-less receive path. It said the send "routes as before", which reads as though the path works; a reply there takes the unstamped `cid…` branch and fails at the group endpoint. Same dead end as before this branch, but a dead end. Reported in review of #5112. Generated-by: Claude Code (Opus 5) --- .../bots/__tests__/dingtalk-bridge.test.ts | 15 ++++- packages/runtime/src/bots/dingtalk-bridge.ts | 56 +++++++++++++------ 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts b/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts index a80115c5dc..989c7a4a2d 100644 --- a/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts +++ b/packages/runtime/src/bots/__tests__/dingtalk-bridge.test.ts @@ -73,14 +73,23 @@ describe('pickDingTalkSendRoute', () => { ); }); - it('keeps unprefixed ids on the group endpoint for pre-existing delivery targets', () => { - // Scheduled-task delivery targets persisted before the prefix existed, - // and ids typed by hand into the scheduled task form. + // Unstamped ids are scheduled-task delivery targets persisted before the + // prefix existed, plus ids typed by hand into the scheduled task form. + // Both outcomes of the pre-stamping discriminator have to survive. + it('keeps an unprefixed conversation id on the group endpoint', () => { const route = pickDingTalkSendRoute(GROUP_CONVERSATION_ID, 'app-key-1', 'hello'); assert.equal(route?.path, '/v1.0/robot/groupMessages/send'); assert.equal(route?.body.openConversationId, GROUP_CONVERSATION_ID); }); + it('keeps an unprefixed staff id on the 1:1 endpoint', () => { + // A bare staff id delivered successfully before stamping existed, so + // routing it to the group endpoint would break a working target. + const route = pickDingTalkSendRoute(SENDER_STAFF_ID, 'app-key-1', 'hi'); + assert.equal(route?.path, '/v1.0/robot/oToMessages/batchSend'); + assert.deepEqual(route?.body.userIds, [SENDER_STAFF_ID]); + }); + it('rejects empty and prefix-only ids', () => { assert.equal(pickDingTalkSendRoute(' ', 'app-key-1', 'hi'), null); assert.equal(pickDingTalkSendRoute('oto:', 'app-key-1', 'hi'), null); diff --git a/packages/runtime/src/bots/dingtalk-bridge.ts b/packages/runtime/src/bots/dingtalk-bridge.ts index cd1bda8d1d..e2655a8ed9 100644 --- a/packages/runtime/src/bots/dingtalk-bridge.ts +++ b/packages/runtime/src/bots/dingtalk-bridge.ts @@ -73,6 +73,13 @@ const DINGTALK_SINGLE_SEND_PATH = '/v1.0/robot/oToMessages/batchSend'; const DINGTALK_GROUP_CHAT_PREFIX = 'group:'; const DINGTALK_SINGLE_CHAT_PREFIX = 'oto:'; +/** + * The discriminator this bridge used before chatIds carried a stamp. + * Retained only to route ids that predate stamping — see the unstamped + * branch of `pickDingTalkSendRoute`. + */ +const DINGTALK_LEGACY_GROUP_ID_PREFIX = 'cid'; + interface DingTalkConnectionOpenResponse { endpoint: string; ticket: string; @@ -154,11 +161,13 @@ export function buildDingTalkSingleSendBody( * Pure helper: route a send to the right DingTalk REST endpoint based on * the chatId prefix that `dingTalkPayloadToEvent` stamps. * - * An unprefixed id is treated as a group `openConversationId`. Those are - * chatIds recorded before this bridge stamped a prefix — persisted - * scheduled-task delivery targets and hand-typed ids in the scheduled - * task form — and group delivery is what they resolved to at the time, - * so keeping that mapping preserves their behavior. + * Unstamped ids are chatIds recorded before this bridge stamped a prefix + * — persisted scheduled-task delivery targets and ids typed by hand into + * the scheduled task form. They keep the pre-stamping discriminator, so + * both of its outcomes survive: a `cid…` conversation id still goes to + * the group endpoint, and a bare staff id still goes to the 1:1 endpoint, + * which is where it was delivering successfully. That guess was only ever + * wrong for a 1:1 *conversation* id, and the stamped path now covers it. */ export function pickDingTalkSendRoute( chatId: string, @@ -178,14 +187,23 @@ export function pickDingTalkSendRoute( body: buildDingTalkSingleSendBody(staffId, robotCode, text), }; } - const openConversationId = targetId.startsWith(DINGTALK_GROUP_CHAT_PREFIX) - ? targetId.slice(DINGTALK_GROUP_CHAT_PREFIX.length).trim() - : targetId; - if (!openConversationId) return null; - return { - path: DINGTALK_GROUP_SEND_PATH, - body: buildDingTalkGroupSendBody(openConversationId, robotCode, text), - }; + if (targetId.startsWith(DINGTALK_GROUP_CHAT_PREFIX)) { + const openConversationId = targetId.slice(DINGTALK_GROUP_CHAT_PREFIX.length).trim(); + if (!openConversationId) return null; + return { + path: DINGTALK_GROUP_SEND_PATH, + body: buildDingTalkGroupSendBody(openConversationId, robotCode, text), + }; + } + return targetId.startsWith(DINGTALK_LEGACY_GROUP_ID_PREFIX) + ? { + path: DINGTALK_GROUP_SEND_PATH, + body: buildDingTalkGroupSendBody(targetId, robotCode, text), + } + : { + path: DINGTALK_SINGLE_SEND_PATH, + body: buildDingTalkSingleSendBody(targetId, robotCode, text), + }; } /** @@ -260,10 +278,14 @@ export function dingTalkPayloadToEvent( const staffId = typeof payload.senderStaffId === 'string' ? payload.senderStaffId.trim() : ''; // Stamp the route while the conversation kind is still known. A 1:1 // reply must address the sender's staff id, not the conversation, so - // that is what the chatId carries. When the payload omits - // `senderStaffId` there is no address to reply to, so fall back to the - // bare conversationId: the message is still delivered upward and keeps - // a stable per-conversation key, and the send simply routes as before. + // that is what the chatId carries. + // + // When the payload omits `senderStaffId` there is nothing a 1:1 reply + // can be addressed to. Fall back to the bare conversationId so the + // message still reaches the agent under a stable per-conversation key, + // but a reply to it will take the unstamped `cid…` branch and fail at + // the group endpoint — the same dead end as before this change, not a + // working path. const chatId = isGroup ? `${DINGTALK_GROUP_CHAT_PREFIX}${conversationId}` : staffId