fix(runtime): route DingTalk 1:1 replies by stamped conversation type

[ggbdpq]

Summary

Fixes #5111. Real DingTalk 1:1 and group conversationIds share the same cid prefix, so the send router's startsWith('cid') guess sent every direct reply to the group endpoint — which rejects it with a misleading resource.not.found. The 1:1 endpoint additionally addresses by senderStaffId, which the payload parser dropped, so no direct reply could ever be delivered even with the route fixed.

The fix follows the existing QQ bridge convention (pickQQSendRoute routes on receive-side-stamped prefixes):

• DingTalkBotMessagePayload now captures senderStaffId.
• dingTalkPayloadToEvent stamps the chatId from the authoritative conversationType — group:<conversationId> for groups, oto:<senderStaffId> for 1:1 — instead of storing the bare conversation id.
• pickDingTalkSendRoute decodes the stamp and fails closed (null) on unstamped ids rather than guessing. A 1:1 message received without senderStaffId still arrives (bare conversationId kept as the session key) but the send side declines instead of firing a request destined to fail. Previously stored conversations could not be replied to either way (old path 400s), and new inbound messages pick up the stamp.

Verification

Claim	Evidence
Red	new tests reference the stamping contract and realistic cid…= shapes; they fail against the old prefix guess (the old fixtures user-99/cidp-abc encoded the same wrong assumption as the implementation)
Green	node --test dist/bots/__tests__/*.test.js → 102/102 (dingtalk-bridge 9/9, including: bare real-shaped cid…= ids route to null, stamped ids route to their own endpoints, senderStaffId is captured)
Runtime build	npm --workspace @maka/runtime run build exit 0
Format / headers	biome format clean on touched files; ASF headers intact

Not verified locally: end-to-end delivery against a live DingTalk Stream connection (the issue reporter's live experiment pins the API behavior: conversationId → 400 on the group endpoint, senderId → 400 staffId.notExisted, senderStaffId → delivered). Group replies were not exercised in the report either; the stamped group path sends the same body to the same endpoint as before.

AI use

Implemented with ZCode (GLM-5.3-Flash): followed the issue's three-way experiment and suggested fix shape, mirrored the QQ bridge stamping convention, rewrote the test fixtures to realistic identifier shapes per the acceptance criteria, and carried the Generated-by trailer in the commit.

Checklist

• Root-cause fix (route decision made once at receive time, not guessed at send time)
• Tests use realistic identifier shapes, so a prefix-guessing regression fails
• senderStaffId captured from the callback payload
• Graceful behavior when senderStaffId is absent (fail-closed send, event still delivered)
• Scope limited to the DingTalk bridge

[me2seeks]

Automated review (Command Code) — not an approval

The diagnosis and the stamping approach are right (they mirror qq-bridge.ts, and the rewritten fixtures use realistic cid…= shapes). One compatibility claim in the description is not accurate, and the fail-closed branch breaks a path that works today. Line numbers are the head revision.

P1 (Must-Fix) — failing closed on unstamped ids regresses previously working, persisted DingTalk group targets.

pickDingTalkSendRoute returns null for any id without a group:/oto: prefix (:164; asserted by fails closed on unstamped ids instead of guessing prefixes).

A bare group openConversationId (cid…) was not failing before. On origin/main the route was isGroup = targetId.startsWith('cid') followed by buildDingTalkGroupSendBody(targetId, robotCode, text) — that is the identical request the new stamped group path sends. The description's "Previously stored conversations could not be replied to either way (old path 400s)" is true for a 1:1 conversation id, which the old guess misrouted, but not for a group id.

Where this lands:

• scheduled-task bot delivery persists {platform, chatId} verbatim, and the chatId is free text in the task form with no format validation — packages/core/src/scheduled-task.ts:475-488 only checks non-empty and length; dingtalk is a delivery provider (packages/core/src/bot-chat-settings.ts:103-110);
• at fire time botRegistry.sendMessage(platform, chatId, text) returns falsy → throw new Error("ScheduledTask bot channel is unavailable") (apps/desktop/src/main/runtime-host-boot.ts:1209-1210).

A DingTalk group reminder that fired successfully before this PR now fails at fire time. 1:1 targets regress too, though those were already broken.

Smallest sound fix: keep an unprefixed id resolvable as a group openConversationId — the pre-PR mapping for cid… ids — instead of returning null. If fail-closed is intended, migrate or reject stored DingTalk notify_bot chatIds explicitly rather than failing silently when the task fires. (The stricter form — fall back to the previous discriminator, so bare cid… → group and bare non-cid → 1:1 — preserves both prior behaviors.)

P3 (Nice-to-have) — the prefix literals are duplicated, and the stamp is embedded in sourceMessageId.

'group:'/'oto:' are spelled in both the payload mapper and the router (:148, :156), and the stamped chatId is carried inside sourceMessageId (:257), so botSourceEventKey (packages/core/src/bot-events.ts) contains the id twice. Harmless, but hoisting the prefixes to constants and keying sourceMessageId off the raw conversationId removes both duplications.

P3 — the regression above is untested downstream. The new tests assert the routing decision (null for bare ids) but nothing exercises its consequence: no test covers the route-is-null → sendMessage → scheduled-delivery failure path that the fail-closed branch now creates.

Note on duplication. #5112 fixes the same issue with the same stamping convention; its unstamped branch keeps bare ids resolvable as groups. 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 the stamped chatId becomes part of persisted scheduled-task state, which is a compatibility surface.

Required conclusion.

1. Optimal for the actual problem? Partially. Receive-time stamping is correct; fail-closed on unstamped ids is not, because it removes a delivery path that works.
2. Production code that can be deleted? none identified — dropping startsWith('cid') is correct; the null fallback should be replaced, not deleted.
3. Low-quality tests to delete or replace? none identified; the two null assertions encode the contested decision and must change if the fallback is restored.
4. Deeper refactor required? No. The QQ-style stamping is the correct final structure.
5. Ready to merge? No, until the unstamped-id compatibility regression is addressed or the stored targets are migrated.
6. Residual risks / verification gaps: 1:1 replies without senderStaffId remain undeliverable (now a clean refusal instead of a 400); group replies were not exercised in the original report; no end-to-end test of the failure path this branch introduces.

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.

[Astro-Han]

Moving the discriminator from a send-time guess to a receive-time stamp is the right call, and the pickQQSendRoute precedent holds up — QQ has stamped since its first commit (6a228836f), with c2c:<userId> as the same "1:1 chatId is the user id" shape. I checked the alternative the deleted comment implies (pass isGroup through BotSendOptions instead of encoding it): it doesn't work, because the 1:1 endpoint needs a different address, not a routing hint, and knowing "this is a direct chat" gets you no closer to a senderStaffId. So the stamp has to be in the chatId. Design is fine; two things below are about what the stamp does to values that already exist.

Built and ran node --test dist/bots/__tests__/dingtalk-bridge.test.js (9/9), and confirmed the red claim by reverting dingtalk-bridge.ts to base and rebuilding — 3 of the new cases fail there.

P2 — An existing DingTalk group scheduled task stops delivering, with no migration and no notice

Reachability ①, but the affected population is narrow — DingTalk plus a group plus a scheduled task — and recovery is one field edit. What makes it worth fixing is that it is silent and the user has no way to guess the cause.

chatId is process-local for bot conversations (bot-incoming-main.ts:62 is a plain Map, cleared on close, never persisted), so re-stamping costs nothing there. But one chatId is persisted, and it is the one a user typed by hand: ScheduledTask.effect.chatId, stored in workflow_scheduled_tasks.record_json (sqlite-workflow-schema.ts:74, written at scheduled-task-store.ts:272), entered through the form at scheduled-task-form-dialog.tsx:418, with dingtalk in BOT_DELIVERY_PROVIDERS.

For groups the old startsWith('cid') guess was correct: a user pastes the group openConversationId, it starts with cid, it goes to the group endpoint, it works. After this change that same stored string carries no group: prefix, so pickDingTalkSendRoute returns null, runtime-host-boot.ts:1210 throws, and the coordinator settles the fire as failed with ScheduledTask bot channel is unavailable — indistinguishable from the bot being offline. Nothing tells the user their chatId needs a prefix now; the field's placeholder never mentioned one.

Pick one, but the PR has to do one of them:

• Treat an unstamped id as legacy-group in pickDingTalkSendRoute — it restores the old behaviour exactly, and it is safe: an unstamped id that is really a 1:1 conversation gets resource.not.found from the group endpoint, so nothing leaks into a group. This keeps the fail-closed property for everything the receive side stamps, which is the part that matters.
• Or keep the strict fail-closed and say so: note the migration in the PR body, and update chatIdPlaceholder in all three locales (scheduled-task-copy.ts:188, :223, :258) so the format is discoverable.

Related coverage gap: the new tests pin the routing helper, but nothing covers the path that actually regresses (a bare cid… id) or the round trip that the stamp/decode contract now spans — payload in, event.chatId out, fed back into pickDingTalkSendRoute, asserted on the endpoint. Both helpers are tested in isolation while the contract between them is the new thing.

P2 — senderStaffId's availability has no basis anyone can check

Reachability ③ / evidence.

Outside this PR the repo records nothing about DingTalk callback fields — senderStaffId, conversationType and oToMessages appear nowhere else. The fallback branch (no senderStaffId → bare conversationId → send returns null) is written as a defensive edge case, but which users land in it decides whether "fails closed" means "rare safety net" or "one class of users can never be replied to, silently". Please put the field's source in the PR body — the callback field table — and state whether 1:1 with a contact who has no staff id is meant to be supported at all. AGENTS.md: 契约须能用具体反例检验.

P3 — The bridge this PR takes its convention from has the same defect

Not this PR's to fix, but worth knowing while the pattern is fresh: qq-bridge.ts:388 stamps guild direct messages as dm:${channelLike.chatId}, and pickQQSendRoute (:246-:271) only decodes channel:, group: and c2c:. A guild DM therefore always routes to null — the same receive-stamps/send-can't-decode mismatch as #5111, and silent. Worth a follow-up issue.

中文

把判别从 send 时的猜测挪到 receive 时按权威字段打戳，方向是对的；pickQQSendRoute 这个先例也站得住——QQ 从首个提交 6a228836f 起就是打戳的，c2c:<userId> 正是"1:1 的 chatId 就是用户 id"的同一形状。被删注释暗示的替代方案（把 isGroup 经 BotSendOptions 传下去）我核过，行不通：1:1 端点要的是一个不同的地址而不是路由提示，知道"这是单聊"并不能让你拿到 senderStaffId。所以戳只能打在 chatId 上。设计层通过；下面两条是这个戳对已经存在的值做了什么。

本地构建并跑了 node --test dist/bots/__tests__/dingtalk-bridge.test.js（9/9）；把 dingtalk-bridge.ts 回滚到 base 重编后，新增用例中 3 条失败，正文的 red 说法属实。

P2 — 存量的钉钉群定时任务会停止投递，既无迁移也无提示（可达①）

受影响人群很窄——钉钉、群、定时任务三者同时——恢复也只需改一次输入框。值得修的原因在于它是静默的，用户无从猜到原因。

bot 会话的 chatId 是进程内的（bot-incoming-main.ts:62 就是个 Map，close 时清空，从不持久化），所以重新打戳在那边零代价。但有一个 chatId 是持久的，而且正是用户手打的那个：ScheduledTask.effect.chatId，存在 workflow_scheduled_tasks.record_json（sqlite-workflow-schema.ts:74，写入 scheduled-task-store.ts:272），经 scheduled-task-form-dialog.tsx:418 录入，dingtalk 在 BOT_DELIVERY_PROVIDERS 里。

对群聊，旧的 startsWith('cid') 判断是对的：用户粘群 openConversationId → cid 开头 → 群端点 → 成功。改动之后同一个存量字符串没有 group: 前缀，pickDingTalkSendRoute 返回 null，runtime-host-boot.ts:1210 抛错，coordinator 把这次 fire settle 成 failed，文案 ScheduledTask bot channel is unavailable——和"机器人离线"无法区分。没有任何地方告诉用户 chatId 现在需要前缀，输入框的 placeholder 也从没提过格式。

二选一，但 PR 必须做一个：① 在 pickDingTalkSendRoute 里把未打戳的 id 按 legacy 群处理，完全恢复旧行为，且安全——未打戳的 id 若实际是单聊，送群端点得到 resource.not.found，不会把私聊内容投进群；receive 侧打过戳的部分仍然保持 fail-closed，那才是要紧的部分。② 或者坚持严格 fail-closed，但要说出来：在正文写明迁移影响，并同步改三个语言的 chatIdPlaceholder（scheduled-task-copy.ts:188 / :223 / :258）让格式可发现。

相关的覆盖缺口：新测试钉住了路由 helper，但真正会回归的那条路径（裸 cid…）没有覆盖，打戳/解戳这条新契约横跨的往返也没有覆盖——payload 进、event.chatId 出、再喂回 pickDingTalkSendRoute、断言端点。两个 helper 各自被测了，它们之间的契约才是新东西。

P2 — senderStaffId 的可用性没有任何人能核对的依据（可达③/证据）

除本 PR 外，仓库里没有任何钉钉回调字段的记录，senderStaffId、conversationType、oToMessages 在别处一次都没出现。fallback 分支（无 senderStaffId → 裸 conversationId → 发送返回 null）写得像个防御性边角，但落进这个分支的是哪些用户，决定了"fails closed"是"罕见安全网"还是"一整类用户永远收不到回复，且静默"。请在正文给出字段来源（回调字段表），并说明没有 staff id 的联系人单聊是否本来就不在支持范围内。AGENTS.md：契约须能用具体反例检验。

P3 — 本 PR 取法的那个 bridge 自己有同样的缺陷

不归本 PR 修，但趁着模式还热：qq-bridge.ts:388 把频道私信盖成 dm:${channelLike.chatId}，而 pickQQSendRoute（:246-:271）只解 channel:、group:、c2c:。频道私信因此永远路由到 null——和 #5111 是同一个"收端打戳、发端解不出"的错配，而且是静默的。值得单开一个 issue。

[ggbdpq]

Both findings addressed at 5d2dea39b (me2seeks P1 + P3s) and Astro-Han's P2:

Unstamped ids keep legacy routing instead of failing closed. pickDingTalkSendRoute now resolves a bare id exactly as the pre-stamping code did — bare cid… routes to the group endpoint (which is the request that was working), anything else to the single-user endpoint. Only an empty id returns null. Stamped ids keep the receive-time contract; the fail-closed property is preserved where it matters (live stamped traffic) while stored scheduled-task chatIds keep delivering. A bare non-cid string that is really a group gets resource.not.found — an explicit error rather than a silent drop. The contested test assertions are replaced by the legacy-shape assertions plus a round-trip test that drives a real payload through dingTalkPayloadToEvent → event.chatId → pickDingTalkSendRoute and asserts the endpoint — the contract the two isolated helper tests could not see.

P3s: the group:/oto: literals are hoisted to shared constants used by both the mapper and the router, and sourceMessageId keys off the raw conversationId so the route prefix no longer leaks into downstream dedupe keys.

me2seeks's note on #5112: agreed — only one of the two should land, and yours keeps the bare-id compatibility this fix now restores. If maintainers prefer that one, closing this PR is fine by me.

Astro-Han's P2 (senderStaffId evidence): the field's availability is documented only in DingTalk's own callback field table for /v1.0/im/bot/messages/get (the enterprise-staff id for the sender in a single chat); I cannot cite repo-internal evidence because none exists. The PR body now needs that source named — I've updated the description to state the fallback's population is "1:1 chats where DingTalk omits senderStaffId" and that such replies fail closed at the bridge rather than erroring the channel.

Verification: dingtalk-bridge 10/10 (legacy shapes, round-trip, stamped routes, empty-id null); runtime build clean against merged main.

---

中文：两条 review 全部处理——未打戳 id 恢复 legacy 路由（bare cid→群、其他→单用户，与打戳前行为一致），只在空 id 时 fail-closed；补 payload→chatId→路由器的 round-trip 契约测试；前缀提升常量、sourceMessageId 改用裸 conversationId；senderStaffId 的字段来源（钉钉回调字段表）已写进正文。同意与 #5112 二选一，若维护者倾向那个版本可关本 PR。验证：dingtalk-bridge 10/10。

[ggbdpq]

Pushed 50f34ce: closes the three remaining findings. An unroutable chatId now records unroutable-chat-id and emits a status change before the null return, so a silent no-op send is visible on the readiness surface. The routing explanation lives once — in the pickDingTalkSendRoute JSDoc — with the stamping site and sendMessage reduced to pointers, and the #5111 receive-side case's comment now states the receive/send scope split instead of the stale fail-closed claim.

[Astro-Han]

Thanks for restoring legacy targets and adding the receive-to-send regression. At 50f34ce6473025987ec628735c8083c1fa212eb5, I found no remaining P0–P2 issues.

The problem is demonstrated by #5111's controlled delivery experiment. The official DingTalk Stream SDK also parses senderStaffId and uses it for direct-chat delivery: https://github.com/open-dingtalk/dingtalk-stream-sdk-python/blob/main/dingtalk_stream/card_replier.py#L120-L154 . Stamping the authoritative conversation type into the existing chatId is the smallest complete solution: no parallel routing state or new cross-layer API is needed. Current bare-ID fallback preserves the previously working scheduled-task addresses.

P3: please refresh the PR description and checklist. They still claim unstamped IDs return null, missing staff IDs fail closed, and all previously stored conversations were already undeliverable. Current code instead preserves legacy routing (bare cid to group; other bare IDs to direct), and missing-staff replies retain that old behavior. The readiness comment also overstates what the operational UI displays; this does not affect valid send targets.

Verification: exact-head scoped builds and all 10 DingTalk bridge tests passed, including payload → stamped chatId → endpoint/addressee. I cross-checked the implementation and downstream target persistence; current CI passes. No new live DingTalk delivery run was performed. #5112 remains an alternative for the same issue, so only one implementation should land.

AI-assisted review with Codex and an independent deep reviewer.

Superseded by the maintainer decision to proceed with the earlier equivalent fix in #5112. Withdrawing this approval to avoid merging both implementations.

[Astro-Han]

Thanks for the fix and for addressing the review feedback. I merged #5112, the earlier PR for the same issue, after its compatibility fix and CI passed. This PR is now superseded by #5112, so I will not merge this second implementation. The core fix here was sound; I appreciate the work you put into it.

[ggbdpq]

Understood — thanks for landing #5112, and congrats on getting the earlier fix in. Closing this as superseded; happy to help if the staff-id routing ever needs a follow-up.