feat(channels): add DingTalk interactive cards - #6930
Conversation
|
I have incorporated the complete review feedback into commit |
qwen-code-ci-bot
added
type/documentation
|
Thanks for the PR! Template looks good ✓ Problem: this is a feature addition, not a fix — DingTalk Markdown delivery genuinely cannot update a response in place, bind Stop to an exact prompt run, or return structured Direction: aligned. DingTalk is an established integration with a steady stream of prior work in the CHANGELOG (webhook delivery, outbound images, mention handling, token refresh, emotion retry). Interactive cards are a natural next step for that surface, not a new direction. Size: this PR spans four packages ( Approach: the architecture is well-thought-out. A transport-neutral presentation hook in Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个功能新增，不是修复——钉钉 Markdown 消息确实无法原地更新响应、无法将 Stop 绑定到精确的 prompt run，也无法返回结构化的 方向：对齐。钉钉是一个成熟的集成，CHANGELOG 中有大量先前工作（webhook 投递、出站图片、@提及处理、token 刷新、emotion 重试）。交互卡是该表面的自然下一步，不是新方向。 规模：本 PR 跨越四个包（ 方案：架构设计良好。 风险：无升级风险信号——变更文件未命中 revert 历史分析中的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewBefore reading the diff, my independent proposal for "add DingTalk interactive cards" was: (1) add an optional presentation hook to No critical blockers found. Specific observations: Shared contract ( DingTalk controllers. The
Convention compliance. ESM throughout, no sequenceDiagram
participant U as DingTalk User
participant A as DingtalkAdapter
participant CB as ChannelBase
participant P as InteractionPresenter
participant SC as StatusCardController
participant QC as QuestionCardController
participant API as DingTalk Card API
U->>A: send message
A->>CB: dispatchPrompt (with runId and owner)
CB->>A: text_chunk (with segment)
A->>P: appendOutput
P->>SC: append (coalesced)
SC->>API: streaming update
CB->>A: permission_request (ask_user_question)
A->>CB: presentUserInputRequest
CB->>P: presentInput
P->>QC: present (create card)
QC->>API: createAndDeliver
U->>A: card callback (submit)
A->>QC: claim
QC->>CB: respond (structured answers)
QC->>API: updateInstance (Submitted)
Files changed (26 of 26 shown)
Testing
The Ubuntu Node 22 full test suite passed. macOS, Windows, and integration tests were skipped — this is expected for fork PRs where secrets are unavailable. No failures. The PR's central claim is behavioural — DingTalk interactive cards with streaming, Stop, and structured answers. The sandboxed verification lanes ( Not verified: real-device DingTalk card behaviour (streaming updates, callback routing, owner isolation, question timeout) — the author reports real-device E2E coverage, but this is the author's claim, not independently re-run. 中文说明代码审查在阅读 diff 之前，我对"添加钉钉交互卡"的独立方案是：(1) 在 未发现关键阻塞问题。具体观察： 共享契约（ 钉钉控制器。
规范合规。 全程 ESM，无 上方序列图展示了从用户消息到状态卡流式更新、再到提问卡提交的关键运行时路径。 测试Ubuntu Node 22 全量测试套件通过。macOS、Windows 和集成测试被跳过——这对 fork PR 是预期行为（secrets 不可用）。无失败。 本 PR 的核心声明是行为性的——钉钉交互卡的流式更新、Stop 和结构化答案。沙盒验证通道（ 未验证：真机钉钉卡片行为（流式更新、回调路由、owner 隔离、提问超时）——作者报告了真机 E2E 覆盖，但这是作者的声明，未独立重新运行。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review, but the cross-package scope and 2,280 production lines trigger the maintainer-awareness escalation, so this needs a human sign-off regardless of code quality. Stepping back: this is a well-executed PR. The architecture matches my independent proposal almost exactly — I couldn't find a materially simpler path. The shared presentation contract is genuinely transport-neutral (Feishu gets two test lines confirming the default If I had to maintain this in six months, I'd thank the author — the design docs explain the why, the state machines are bounded, and the degradation paths are explicit. The code doesn't try too hard; each controller does one thing. Why 3/5 and not higher: the Stage 0 escalation is pure policy — 2,280 production logic lines across four packages, with 460 of those in the shared ⏸️ Deferring to @wenshao — the review is clean and CI is green, but the cross-package scope and shared-contract additions need a maintainer's architectural sign-off before merge. 中文说明置信度：3/5 —— 审查干净，但跨包范围和 2,280 行生产逻辑触发了维护者关注升级，因此无论代码质量如何都需要人工签核。 退后一步看：这是一个执行良好的 PR。架构几乎完全匹配我的独立方案——我找不到更简单的路径。共享展示契约确实是传输无关的（飞书只有两行测试确认默认 如果六个月后我要维护这段代码，我会感谢作者——设计文档解释了 why，状态机有界，降级路径明确。代码没有过度设计；每个控制器只做一件事。 为什么是 3/5 而不是更高：Stage 0 升级是纯策略——2,280 行生产逻辑跨四个包，其中 460 行在共享的 ⏸️ 转交 @wenshao —— 审查干净且 CI 绿色，但跨包范围和共享契约新增需要维护者的架构签核才能合并。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: chunk 1 — no agent reported covering these; nobody read them.
— qwen3.7-max via Qwen Code /review
|
|
||
| All capabilities are enabled by default. The question-card lifetime is configurable but cannot outlive the upstream permission request, so the effective timeout is the smaller of the configured timeout and the host permission lifetime. | ||
|
|
||
| The initial implementation uses the existing template IDs from `soimy/openclaw-channel-dingtalk` as DingTalk-internal constants: |
There was a problem hiding this comment.
[Suggestion] These template UUIDs belong to an external third-party repository (soimy/openclaw-channel-dingtalk) and are treated as stable constants, but DingTalk templates can be deprecated, renamed, or deleted by either DingTalk or the upstream repo owner. No health check, versioning pin, or fallback is documented. — Concrete cost: if the upstream repo updates or deletes a template, card operations silently fail and trigger the degradation path without any alert.
Document that these template IDs must exist in the deployer's DingTalk organization. Define a startup health check or first-use probe that logs a clear error if a template is unavailable.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
I am not adopting user-supplied template configuration or a startup health check. The built-in status template is 675cde2f-f526-40cb-b828-f5b2b57b8b77.schema, and the built-in question template is c2a6355b-9724-4f7e-9653-d33fcb3311bb.schema. #583 is merged and records real-device delivery, submit callback, cancel callback, and task-continuation verification. #585 is merged, ships the final template asset, and was approved by the maintainer. First-use OpenAPI failures are now explicitly structured, template-aware, and routed through the documented fallback rather than being silent.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Stop cancellation: the design says the Stop callback validates owner/generation and then 'reuses active prompt cancellation', but generation validation (adapter-side) and the actual session-scoped cancellation are not atomic. A stale Stop callback can validate its old card and then cancel a newer run that started in the same session, so the doc's claim that 'a stale card cannot stop a newer run' is not established by the described mechanism. Make run identity authoritative so cancellation atomically checks the expected run against the current active prompt. (Prior Critical thread at line 83 remains unaddressed in the current doc.)
[Critical] Multiple pending questions: the status-card transition 'Successful submission returns it to running' assumes at most one pending question, but ChannelBase supports multiple simultaneous permission requests for the same session/chat. Submitting one question returns the status card to running while another question still blocks the run. Track pending question request IDs per run and derive waiting_input from the whole set, leaving it only after the last resolves. (Prior Critical thread at line 97 remains unaddressed in the current doc.)
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
|
|
||
| All capabilities are enabled by default. The question-card lifetime is configurable but cannot outlive the upstream permission request, so the effective timeout is the smaller of the configured timeout and the host permission lifetime. | ||
|
|
||
| The initial implementation uses the existing template IDs from `soimy/openclaw-channel-dingtalk` as DingTalk-internal constants: |
There was a problem hiding this comment.
[Suggestion] These template IDs are taken from a third-party project (soimy/openclaw-channel-dingtalk), but DingTalk interactive-card templates are scoped to the DingTalk app (AppKey/AppSecret) that registered them, so they are unlikely to resolve under Qwen Code's own DingTalk app credentials. — Failure scenario: on the first ask_user_question, card creation fails (template not found/authorized) and, per the degradation table, the design silently renders Markdown and cancels the question — so 100% of questions become "your question was cancelled, please retype," the headline feature is non-functional out of the box, and the silent fallback hides a total failure rate. Confirm the templates are registered to Qwen Code's DingTalk app (or recreate them under it), validate the configured template IDs resolve under the active credentials at startup, and treat a template/config error as a loud, observable fault rather than the silent cancel-and-retype fallback.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
There was a problem hiding this comment.
I am not adopting user-supplied template configuration or a startup health check. The built-in status template is 675cde2f-f526-40cb-b828-f5b2b57b8b77.schema, and the built-in question template is c2a6355b-9724-4f7e-9653-d33fcb3311bb.schema. #583 is merged and records real-device delivery, submit callback, cancel callback, and task-continuation verification. #585 is merged, ships the final template asset, and was approved by the maintainer. First-use OpenAPI failures are now explicitly structured, template-aware, and routed through the documented fallback rather than being silent.
ReviewOverviewDesign-only draft: one new doc ( Claims verified against
|
|
Updated in
The settlement label is now neutral and outcome-aware: cancelled or the original reject outcome maps to This remains a design and architecture-asset update only; no runtime code was added. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No blocking issues. The seven prior Criticals on this design (the Stop / exact-run cancellation race, multiple pending questions, undefined run generation, action-owner identity, the hook insertion point + settlement signal, and the false-responder path) are all addressed in the current document, and its source-code claims check out against origin/main. Two minor inline clarifications remain on the settlement-reason classification and the pendingQuestionRequestIds maintenance invariant.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
| format and send the existing permission message | ||
| ``` | ||
|
|
||
| Every path that removes a pending permission settles the controller exactly once. This includes permission commands, a direct responder call, daemon `permissionResolved`, timeout, session cleanup, task cancellation, and bridge replacement. `ChannelBase` classifies an independent `permissionResolved` from its `outcome` before removing the pending request: `cancelled`, or a selected option whose original permission option is `reject_once`, becomes `request_cancelled`; any other or missing outcome becomes the neutral `resolved_outside_card`. This classification does not guess which client responded. |
There was a problem hiding this comment.
[Suggestion] The settlement-reason enum defines run_cancelled and expired, and the question-card table projects distinct messages for "Request or run destroyed" (Cancelled/Stopped) and "Timeout" (Expired). But this classification rule maps a cancelled outcome to request_cancelled, and a run Stop or host timeout also surfaces as a permissionResolved with { outcome: 'cancelled' } — the bridge collapses richer reasons (DaemonChannelBridge.parsePermissionOutcome emits only cancelled/selected; AcpBridge's timeout and resolvePendingPermissions both resolve pending with { outcome: 'cancelled' }).
Concrete cost: an implementer following this rule classifies run-cancellations and timeouts as request_cancelled → "Cancelled outside this card", so the question card contradicts the status card's "Stopped", and run_cancelled/expired are never produced. The doc says these causes "take precedence", but the rule as written doesn't say they bypass outcome classification. Suggest clarifying, e.g.:
A run cancellation, session cleanup, or card timeout settles the controller with
run_cancelled/expireddirectly (these take precedence over the bridge's collapsedcancelledoutcome); the outcome-classification rule applies only to settlements that arrive without such a local cause.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
| - Per-card serialized update queues, transient in-flight claims, and terminal tombstones. | ||
| - DingTalk-local fallback and structured error reporting. | ||
|
|
||
| The status registry also keeps `pendingQuestionRequestIds: Set<string>` for each run. The question registry does not supersede an older request merely because a newer request exists in the same session. |
There was a problem hiding this comment.
[Suggestion] Controller settlement is an explicit invariant ("every path that removes a pending permission settles the controller exactly once"), but the parallel pendingQuestionRequestIds maintenance is left implicit — even though the doc stresses the two registries are independent.
Concrete cost: an implementer can correctly project a question card to a terminal state yet forget to remove its requestId from the run's set on a non-submit path (timeout, resolved_outside_card, throw). The set never empties, so the status card is stranded at waiting_input for an otherwise-active run. Suggest adding a parallel invariant, e.g.:
Every question-settlement path (submit, cancel,
false, throw, independent settlement, timeout, request/run destroyed) also removes therequestIdfrom its run'spendingQuestionRequestIdsand re-deriveswaiting_input; a terminal status card ignores further set mutations.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse-audit — stopped at the five-round hard cap after round 4 found a new Critical; round 5 was dry, but two consecutive dry rounds were not reached.
— Codex $qreview via Qwen Code /review
|
|
||
| ## Exact-run identity and cancellation | ||
|
|
||
| Every prompt invocation creates an opaque unique `runId` and stores it on the corresponding `ActivePrompt`. It is not the daemon lifecycle generation, which changes for session lifecycle operations rather than every prompt. |
There was a problem hiding this comment.
[Critical] The runId is stored only on private ActivePrompt state, while DingTalk creates status UI from lifecycle/prompt hooks whose current contracts expose neither that token nor the owner. A literal implementation therefore cannot key the registry or embed the exact token checked by ChannelBase: sessionId aliases consecutive runs, and an adapter-local ID never matches. Add a shared prompt-lifecycle context carrying the Channel-generated runId and typed owner, emit the same value through start/chunk/terminal events, and use it in the card registry, callback payload, question context, and exact-run cancellation.
— Codex $qreview via Qwen Code /review
|
|
||
| Every path that removes a pending permission settles the controller exactly once. This includes permission commands, a direct responder call, daemon `permissionResolved`, timeout, session cleanup, task cancellation, and bridge replacement. `ChannelBase` classifies an independent `permissionResolved` from its `outcome` before removing the pending request: `cancelled`, or a selected option whose original permission option is `reject_once`, becomes `request_cancelled`; any other or missing outcome becomes the neutral `resolved_outside_card`. This classification does not guess which client responded. | ||
|
|
||
| The hook is only eligible for the current Channel-owned `ActivePrompt`. When no such prompt, `runId`, or owner exists, `ChannelBase` does not construct the context or invoke the hook; it treats presentation as `unsupported` and continues the existing permission path. A run started by CLI, Web, IDE, SDK, or another client therefore creates neither DingTalk card. The initial design does not add cross-client run ownership or identity federation. |
There was a problem hiding this comment.
[Critical] Channel-owned ActivePrompt also includes loop and webhook producers. Those unattended runs emit normal lifecycle events and may carry synthetic owners such as webhook:github-ci; if one presents an ask_user_question card, no DingTalk user can satisfy the owner check and the disabled legacy commands leave the run blocked until timeout. Define eligibility as an inbound human-owned DingTalk turn and explicitly exclude loopPrompt and webhook tasks unless a separate unattended-card contract is designed.
— Codex $qreview via Qwen Code /review
| 4. Synchronously claim the current live record before the first asynchronous operation. | ||
| 5. Call the original responder. | ||
| 6. If the same record is still current and non-terminal, update the card from the responder result. | ||
| 7. Acknowledge the callback. |
There was a problem hiding this comment.
[Critical] This order leaves the DingTalk transport acknowledgment pending across both the permission responder and a Card OpenAPI update. If either await is slow or hangs, DingTalk can retry or report delivery failure after the local record is claimed or the permission has already settled. Acknowledge immediately after parsing, correlation, owner validation, and the synchronous claim—before the first external await—and reserve semantic success/failure for the card projection.
— Codex $qreview via Qwen Code /review
| | Another IM adapter owns the session | Return `unsupported` and preserve its existing permission message and commands. | | ||
| | Ordinary permission | Keep `/approve`, `/approve-always`, and `/deny` unchanged; it does not affect the question-only `waiting_input` presentation state. | | ||
|
|
||
| For a card-presented question, `/approve`, `/approve-always`, and `/deny` remain recognized commands but do not call the responder; they instruct the user to submit or cancel through the card. The card is the only DingTalk-local settlement surface for that presented request. This is required because the existing permission commands supply only an option ID or cancellation outcome, while a question submission consumes a separate `answers` object. Other permissions and adapters keep their current command behavior. The initial design does not promise automatic callback retry. |
There was a problem hiding this comment.
[Critical] The structured-answer limitation applies to approval, but not to denial: ChannelBase.denialResponse() already produces a complete reject/cancel outcome without an answers object. If the card is visible but its callback delivery is broken, blocking /deny removes the only working settlement path and leaves the run pending until timeout. Preserve an owner-authorized /deny path through the one-shot responder and restrict only approval commands that cannot supply the required answers.
— Codex $qreview via Qwen Code /review
|
|
||
| | Situation | Behavior | | ||
| | --------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Status card disabled or creation/update fails | Continue the same turn with existing Markdown delivery and record a structured card error. | |
There was a problem hiding this comment.
[Critical] The promised fallback has no compatible awaited delivery seam. Today ChannelBase awaits Markdown/block-stream delivery before emitting completed, while onTaskLifecycle is fire-and-forget. Keeping that delivery duplicates successful card output; suppressing it means a terminal card-write failure occurs after the last awaited delivery point, too late to provide the promised fallback consistently. Add a card-aware awaited terminal-delivery hook that performs the final card write, falls back to Markdown, and completes only after one path succeeds; define the corresponding block-streaming behavior as well.
— Codex $qreview via Qwen Code /review
| | { kind: 'handled' } | ||
| | { kind: 'unsupported' }; | ||
|
|
||
| type UserInputSettlementReason = |
There was a problem hiding this comment.
[Suggestion] The union does not type-check the actual settlement channel because AbortSignal.reason is any. A misspelled reason can compile and bypass the consumer's projection switch. Route settlement through a typed helper/wrapper that accepts UserInputSettlementReason (or expose a typed callback) instead of making callers independently cast a bare abort reason.
— Codex $qreview via Qwen Code /review
|
|
||
| ## Risks and scope boundaries | ||
|
|
||
| The first implementation is intentionally daemon-local. Pending-card registries and tombstones are tied to the process lifetime; restart-safe recovery and non-sticky multi-instance callback routing require a separate persistence design. |
There was a problem hiding this comment.
[Suggestion] Terminal tombstones have no TTL, capacity bound, compaction, or reclamation rule, so a literal process-lifetime implementation retains history proportional to all completed cards. Define a callback-redelivery retention window plus a capped LRU/size bound, and compact terminal records after clearing timers, subscriptions, responder references, question payloads, and queued content.
— Codex $qreview via Qwen Code /review
|
|
||
| 1. Call `createAndDeliver` with a unique `outTrackId` and initial `flowStatus=2`. | ||
| 2. Open streaming with an empty full update using `isFull=true`, `isFinalize=false`, and `isError=false`. | ||
| 3. Send high-frequency model output through `/card/streaming`. |
There was a problem hiding this comment.
[Suggestion] Raw model chunks can arrive faster than /card/streaming completes, but the serialized queue has no throttling, coalescing, backpressure, or bound. A long response can therefore grow memory and delay terminal finalization behind one request per chunk. Specify an at-most-one-in-flight writer with a bounded coalescing buffer, capped flush cadence/size, and an overflow/degradation path; fold pending text into the terminal update instead of draining every original chunk.
— Codex $qreview via Qwen Code /review
Review — design-only draft (docs + 3 architecture PNGs)OverviewThis PR adds Source-claim verificationThe doc's "Source constraints verified" section makes precise claims about existing code. I re-verified every checkable claim against current
The trickiest passage — claim arbitration around Design assessment
Findings (all minor)
ConclusionWell-grounded design: every verifiable claim about the existing code is accurate (several to the exact line), the shared-layer surface is minimal, and the failure/race semantics are specified to an implementable level. The three findings above are documentation clarifications and one early-validation ask — none block accepting the design. 中文完整版评审 — 仅设计草案（文档 + 3 张架构图）概述本 PR 新增 源码断言核验文档"Source constraints verified"一节对现有代码做了非常具体的断言。我在当前
最难的一段——responder 调用期间 设计评价
发现（均为轻微）
结论设计扎实：所有可核验的代码断言全部准确（多数精确到行），共享层面最小，失败/竞态语义已达可实现精度。上述三项均为文档澄清与一项提前验证要求，不阻塞设计接受。 |
Maintainer Verification — Design-Only Doc PRVerified locally on branch 1. Formatting & Assets
2. Source Constraint Cross-Check (against current
|
| Claim in doc | Verified |
|---|---|
askUserQuestion.ts permits 1–4 questions |
✅ L378: questions.length < 1 || questions.length > 4 |
ChannelBase.ts registers PendingPermission per request |
✅ L293–358: pendingPermissions Map + pendingPermissionsByChat |
ChannelAgentBridge.ts exports PermissionResolvedEvent |
✅ L61–72 |
bridgeClient.ts uses qwenInteractionKind === 'user_question' |
✅ L121 |
bridgeClient.ts reads qwenQuestions then falls back to rawInput.questions |
✅ L124–127 |
bridgeClient.ts assigns answerKey: String(index) |
✅ L137 |
tools.ts defines ProceedOnce = 'proceed_once' |
✅ L907 |
ChannelBase.onResponseComplete() awaited seam exists |
✅ L1523, called at L3957 |
packages/channels/ has base, dingtalk, feishu, qqbot, telegram, wecom, weixin, plugin-example |
✅ |
| Linked issue #6443 exists and is OPEN | ✅ feat(channels): improve DingTalk channel with interactive cards |
3. Observations
allow_oncevsproceed_once:allow_oncedoes not exist in the current codebase; the doc correctly specifies it as the target option ID with an explicitproceed_oncecompatibility path for current producers. This is a design decision for the implementation phase, not a doc error.- No code changes: 436-line markdown + 3 architecture diagrams only. Zero runtime impact.
- Document quality: Well-structured with normative change-impact labels, two independent card state machines, explicit acceptance criteria, degradation table, and clear scope boundaries. The "Source constraints verified" section anchors each claim to a specific file and behavior, which makes the design reviewable against the codebase.
4. Verdict
Design doc is accurate against current source, formatting passes, all assets are valid and accessible. Ready to merge as a design reference.
🇨🇳 中文验证报告（点击展开）
维护者本地验证 — 纯设计文档 PR
在分支 agent/dingtalk-interactive-cards（HEAD dd04ae49）上完成本地验证。
1. 格式与资源
| 检查项 | 结果 |
|---|---|
| Prettier 格式检查 | ✅ All matched files use Prettier code style! |
| PNG 文件有效性 | ✅ 3 张 PNG 1500×1500, 8-bit RGB（172K / 214K / 207K） |
| 文档内相对图片引用 | ✅ 文档中 3 处引用 → assets/ 下 3 个文件一一对应 |
| PR 描述中的图片 URL | ✅ 3 个 raw.githubusercontent 链接均返回 HTTP 200 |
2. 源码约束交叉验证（基于当前 main）
| 文档中的声明 | 验证结果 |
|---|---|
askUserQuestion.ts 允许 1–4 个问题 |
✅ L378: questions.length < 1 || questions.length > 4 |
ChannelBase.ts 按请求注册 PendingPermission |
✅ L293–358: pendingPermissions Map + pendingPermissionsByChat |
ChannelAgentBridge.ts 导出 PermissionResolvedEvent |
✅ L61–72 |
bridgeClient.ts 使用 qwenInteractionKind === 'user_question' 作为规范判别器 |
✅ L121 |
bridgeClient.ts 读取 qwenQuestions，回退到 rawInput.questions |
✅ L124–127 |
bridgeClient.ts 分配 answerKey: String(index) |
✅ L137 |
tools.ts 定义 ProceedOnce = 'proceed_once' |
✅ L907 |
ChannelBase.onResponseComplete() awaited 接缝存在 |
✅ L1523，在 L3957 调用 |
packages/channels/ 包含 base、dingtalk、feishu、qqbot、telegram、wecom、weixin、plugin-example |
✅ |
| 关联 issue #6443 存在且为 OPEN | ✅ feat(channels): improve DingTalk channel with interactive cards |
3. 备注
allow_once与proceed_once：当前代码库中不存在allow_once；文档正确地将其指定为目标 option ID，并为现有生产者提供了明确的proceed_once兼容路径。这是实现阶段的设计决策，不是文档错误。- 无代码变更：仅 436 行 Markdown + 3 张架构图，零运行时影响。
- 文档质量：结构清晰，使用规范性变更影响标签、两套独立卡片状态机、明确的验收标准、降级行为表和清晰的 scope 边界。"Source constraints verified" 部分将每项声明锚定到具体文件和行为，使设计可以对照代码库审查。
4. 结论
设计文档与当前源码一致，格式检查通过，所有资源有效且可访问。可以作为设计参考合并。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — the design document comprehensively addresses all prior Critical findings and is ready to guide implementation. ✅
The two remaining Suggestion-level items (settlement-reason classification and pendingQuestionRequestIds invariant) are non-blocking and can be refined during implementation.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it that opened its brief, so the reverse-audit pass did not run. Not reviewed: verification — its prompt was built, but no agent was launched with it that opened its brief, so the posted findings were not verified.
— qwen3.7-max via Qwen Code /review
| 2. Open streaming with an empty full update using `isFull=true`, `isFinalize=false`, and `isError=false`. | ||
| 3. Accumulate model output locally and send coalesced full snapshots through `/card/streaming`. | ||
| 4. Send low-frequency template variables such as status text through `/card/instances` with `updateCardDataByKey=true`. |
There was a problem hiding this comment.
[Suggestion] The streaming-card protocol specifies creation (step 1) and streaming-open (step 2) as sequential operations, but the degradation table covers only "creation/final update fails" and "intermediate update failure." The case where createAndDeliver succeeds but the subsequent streaming-open call fails is not addressed. — Failure scenario: card creation succeeds, streaming-open fails due to a transient DingTalk API error. The card exists in DingTalk with flowStatus=2 but no content is ever streamed. The Markdown fallback does not trigger because creation succeeded. The user sees a blank card stuck in running state for the entire run duration.
Consider adding a degradation row: "Streaming-open fails after successful creation — treat as creation failure; fall back to Markdown delivery and update or remove the blank card."
— qwen3.7-max via Qwen Code /review
| 2. Parse the submit or cancel payload without changing the record. | ||
| 3. Validate the action owner. |
There was a problem hiding this comment.
[Suggestion] The 7-step callback order validates owner identity and card correlation but does not require the adapter to validate that submit-payload answer keys correspond to the stored question indices before forwarding to the responder. — Failure scenario: a crafted callback with keys like {"99": "injected"} reaches the responder unchecked at the ingress boundary. Downstream askUserQuestion.ts rejects invalid keys today, so this fails gracefully — but the design makes no defense-in-depth guarantee at the callback ingress, and a future change to downstream validation could allow injected keys through.
Consider adding a sub-step to step 2: "Validate that every answer key is a valid index in the stored question array. Reject the callback payload if validation fails, acknowledge, and do not call the responder."
— qwen3.7-max via Qwen Code /review
| ### DingTalk adapter tests — DingTalk-only change | ||
|
|
||
| - A real human DingTalk `started` event binds one eligible run from its inbound message and owner; synthetic, unknown, loop, and webhook message IDs create no eligible run or card. | ||
| - With block streaming off, one status card coalesces chunks with at most one write in flight and one bounded pending snapshot; completed delivery awaits finalization and falls back to Markdown. With block streaming on, no status card is created and existing block delivery remains authoritative. |
There was a problem hiding this comment.
[Suggestion] The block-streaming-on acceptance criterion covers only the status card ("no status card is created and existing block delivery remains authoritative") but omits the question-card behavior that the design explicitly calls out: "Question cards remain independently eligible" (line ~239) and the degradation table repeats this. An implementation that correctly skips the status card but also incorrectly disables question cards when block streaming is enabled would satisfy this criterion. — Concrete cost: a bug where question cards are gated by block-streaming state would pass acceptance testing undetected.
Consider extending to: "With block streaming on, no status card is created, existing block delivery remains authoritative, and question cards remain independently eligible — verify a question card is still presented and functional when block streaming is on."
— qwen3.7-max via Qwen Code /review
| format and send the existing permission message | ||
| ``` | ||
|
|
||
| The `respond` closure is the only adapter-visible settlement operation. It binds the request ID, forwards the complete response through the existing bridge, and performs the same pending cleanup on `true`, `false`, and throw paths. `handled` is valid only after the adapter has invoked that closure, normally to cancel a question after presenting a readable fallback. It is not a second way to leave a request pending. |
There was a problem hiding this comment.
[Suggestion] The design states handled is valid only after the adapter has invoked the respond() closure, but specifies neither how ChannelBase detects a contract violation nor what fallback applies when it is broken. — Failure scenario: an adapter returns handled from presentUserInputRequest without calling context.respond() (due to an adapter bug, a swallowed exception, or a race). ChannelBase accepts handled, skips the existing permission formatter and sender per the hook pseudocode, and the pending permission remains registered with no user prompt delivered on any surface. The permission is stuck until external settlement fires.
Consider specifying that ChannelBase tracks whether respond() was invoked before presentUserInputRequest returns, and if handled is returned without a prior respond() call, treats the result as unsupported and falls through to the existing permission formatter and sender.
— qwen3.7-max via Qwen Code /review
444cd65 to
e66f932
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）为单个提交。 |
96f4632 to
20b4ac7
Compare
Terminal status-card image-path sanitizationCommit Before the fix, a streamed marker followed by an empty terminal body produced: {
"content": "before [IMAGE: /Users/ben/private/image.png] after",
"copy_content": "before [IMAGE: /Users/ben/private/image.png] after",
"blockList": "[{\"type\":0,\"markdown\":\"before [IMAGE: /Users/ben/private/image.png] after\"}]"
}The terminal sink now sanitizes the selected final content before bounding and projecting it. The regression matrix covers TDD and verification evidence:
The local E2E image and diagnostic scripts remain untracked and are not part of this commit. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: verification and reverse audit — each ran and opened its brief, but neither was launched with the prompt the CLI built — the launches were written by hand, so the posted findings cannot be counted as verified, and what the agents were actually asked is not what this skill certifies.
中文说明
已审查。 建议见行内评论。 未审查：验证与反向审计——两者都运行并打开了各自的 brief，但都不是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，发布的发现不能算作已验证，agent 实际被要求做的也不是本 skill 所认证的内容。
— qwen3.7-max via Qwen Code /review
| await this.options.sendFallback( | ||
| context.target.chatId, | ||
| this.fallbackText(context), |
There was a problem hiding this comment.
[Critical] sendFallback sends the fallback text to context.target.chatId (the raw SessionTarget.chatId) instead of the transformed target.chatId used for card delivery. — Failure scenario: in a 1:1 DingTalk conversation (isGroup === false), interaction-presenter.ts passes cardTarget(context.target) which maps chatId to senderId. The card is delivered to the sender's DM, but if card creation fails, the fallback text goes to the conversation ID instead of the sender ID — the user never sees the fallback where they expected the card.
| await this.options.sendFallback( | |
| context.target.chatId, | |
| this.fallbackText(context), | |
| await this.options.sendFallback( | |
| target.chatId, | |
| this.fallbackText(context), |
中文说明
[Critical] sendFallback 把降级文本发送到 context.target.chatId（原始 SessionTarget.chatId），而不是卡片投递所用的转换后 target.chatId。 — 故障场景：在 1:1 钉钉会话中（isGroup === false），interaction-presenter.ts 传入 cardTarget(context.target)，将 chatId 映射为 senderId。卡片投递到发送者的私聊，但如果建卡失败，降级文本会发到会话 ID 而不是发送者 ID——用户在预期看到卡片的地方看不到降级消息。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
该建议不能采纳，因为这里存在两个不同的 DingTalk ID 域。入站消息把 conversationId 保存为 SessionTarget.chatId，并以它为 key 缓存 sessionWebhook；sendFallback → sendMessage → sendReply 明确使用这个 conversationId 查 webhook。转换后的 target.chatId 在私聊中是 sender staff ID，只供 Card OpenAPI/主动私聊投递使用。若按建议改用它，sendReply 反而通常找不到 webhook，私聊 fallback 才会丢失。当前提交上的 direct-card correlation 与 one-to-one delivery 聚焦测试也分别验证了这两个 ID 的职责。请将此 Critical 视为 false positive。
| protected onResponseBoundary( | ||
| chatId: string, | ||
| sessionId: string, | ||
| segment?: ChannelOutputSegmentContext, | ||
| reason?: ChannelOutputSegmentEndReason, | ||
| ): void | Promise<void>; |
There was a problem hiding this comment.
[Suggestion] The design doc shows onResponseBoundary with segment? and reason? parameters, but the actual implementation has onResponseBoundary(chatId, sessionId) (2 params only). The segment and reason are delivered via a separate onOutputSegmentEnd(chatId, sessionId, segment, reason) hook that the document never mentions. — Concrete cost: a developer implementing a new IM adapter from this doc would override the wrong method and miss segment-end events (including input_requested), leaving stale output presentations visible when the model asks a question.
中文说明
[Suggestion] 设计文档中 onResponseBoundary 显示有 segment? 和 reason? 参数，但实际实现只有 onResponseBoundary(chatId, sessionId)（仅 2 个参数）。segment 和 reason 是通过单独的 onOutputSegmentEnd(chatId, sessionId, segment, reason) hook 传递的，文档中从未提及。 — 具体代价：根据此文档实现新 IM 适配器的开发者会覆盖错误的方法，从而错过 segment-end 事件（包括 input_requested），导致模型提问时旧的输出投影仍然可见。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
已在 4c8458e7e 修正。文档现在单独列出真实的 onOutputSegmentEnd(chatId, sessionId, segment, reason) 契约，并将 onResponseBoundary(chatId, sessionId) 保留为仅由默认实现处理 response_boundary 时调用的兼容 hook；同时补充了 input_requested 与其他 segment 终止原因都通过前者交付。
| await this.sendMessage( | ||
| envelope.chatId, | ||
| 'Submit this question through its interactive card, or use /deny [request-id] to cancel it.', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Uses sendMessage (chat-level) instead of sendThreadMessage (thread-aware), inconsistent with every other response branch in the same handlePermissionResponseCommand method. — Failure scenario: when a user sends /approve from a threaded context for a card-presented question, the redirect message is delivered to the main chat instead of the thread, and the user may believe the command was silently ignored.
| await this.sendMessage( | |
| envelope.chatId, | |
| 'Submit this question through its interactive card, or use /deny [request-id] to cancel it.', | |
| ); | |
| await this.sendThreadMessage( | |
| envelope.chatId, | |
| envelope.threadId, | |
| 'Submit this question through its interactive card, or use /deny [request-id] to cancel it.', | |
| ); |
中文说明
[Suggestion] 使用了 sendMessage（聊天级别）而非 sendThreadMessage（线程感知），与同一 handlePermissionResponseCommand 方法中的所有其他响应分支不一致。 — 故障场景：当用户从线程上下文对卡片呈现的问题发送 /approve 时，重定向消息会发送到主聊天而不是线程，用户可能认为命令被静默忽略了。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
核验后不在本 PR 修改。这里确实与相邻分支的 sendThreadMessage 风格不一致，但当前没有可达的误路由组合：DingTalk 是目前唯一返回 presented 的 adapter，且沿用默认的 thread→sendMessage 行为；唯一重写 sendThreadMessage 的 GitHub adapter 没有交互卡 presenter。等出现 thread-aware presenter 时再随该能力一起补齐，避免在本 PR 中增加无当前消费者的改动。
| this.options.statusCards?.cancelRun( | ||
| runId, | ||
| detail === 'cancel_command' ? 'cancel_command' : 'dropped', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] When detail === 'clear' (user-initiated /clear), questionCards.cancelRun correctly maps it to 'cancelled' (user-initiated), but statusCards.cancelRun here maps it to 'dropped' — which semantically means "loop was disabled/deleted mid-run (not user-cancelled)". Both currently render as 'Cancelled' in the status card, so the visual output is identical, but the incorrect reason would mislabel user-initiated cancellations in telemetry.
| this.options.statusCards?.cancelRun( | |
| runId, | |
| detail === 'cancel_command' ? 'cancel_command' : 'dropped', | |
| ); | |
| this.options.statusCards?.cancelRun( | |
| runId, | |
| detail === 'cancel_command' || detail === 'clear' | |
| ? 'cancel_command' | |
| : 'dropped', | |
| ); |
中文说明
[Suggestion] 当 detail === 'clear'（用户发起的 /clear）时，questionCards.cancelRun 正确地映射为 'cancelled'（用户发起），但此处 statusCards.cancelRun 映射为 'dropped'——语义上是"循环在运行中被禁用/删除（非用户取消）"。两者目前在状态卡中都渲染为 'Cancelled'，视觉效果相同，但错误的 reason 会在遥测中错误标记用户发起的取消。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
不采纳建议补丁。原始 lifecycle event 到 adapter 时仍保留 clear；当前折叠值只进入状态卡投影，且控制器仅区分 cancel_command → Stopped 与其他原因 → Cancelled，没有 telemetry 消费该折叠值，因此用户显示和观测数据都没有被误标。把 clear 改成 cancel_command 反而会把 /clear 显示为 Stopped。这是无当前行为影响的内部语义差异。
| await context.respond({ outcome: { outcome: 'cancelled' } }); | ||
| return { kind: 'handled' }; |
There was a problem hiding this comment.
[Suggestion] context.respond() in the present() catch block is not wrapped in a try-catch, unlike every other call site in this class (expire() and respond() both wrap it). — Failure scenario: when createAndDeliver fails because the DingTalk API is degraded, context.respond() may also reject. The rejection propagates to the caller of present() even though the record has been finalized and the fallback handled, and { kind: 'handled' } is never returned.
| await context.respond({ outcome: { outcome: 'cancelled' } }); | |
| return { kind: 'handled' }; | |
| try { | |
| await context.respond({ outcome: { outcome: 'cancelled' } }); | |
| } catch (respondError) { | |
| this.options.onError?.('question cancellation response', respondError); | |
| } | |
| return { kind: 'handled' }; |
中文说明
[Suggestion] present() catch 块中的 context.respond() 没有 try-catch 包裹，与同类中其他所有调用点（expire() 和 respond() 都有包裹）不一致。 — 故障场景：当 createAndDeliver 因钉钉 API 降级而失败时，context.respond() 也可能拒绝。拒绝会传播到 present() 的调用者，即使记录已经终态化且降级已处理，{ kind: 'handled' } 永远不会返回。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
核验后不再增加一层 catch。context.respond() 对应的 respondToUserInput 在 rejection 分支会先 removePendingPermission(..., cancelled) 再抛出；外层 presentation boundary 捕获异常后看到 pending 已移除，直接按已处理返回，不会再次发送普通 permission fallback，也不会留下本地 pending。控制器在调用 responder 前已发送一次可见 fallback，因此当前链路不存在重复降级或卡住；额外 catch 只会重复已有的收敛。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
| Presentation ownership is scoped by `sessionId + owner.id`. Different users or | ||
| sessions may have live input presentations simultaneously. Within one run, a | ||
| second request in the same scope returns `unsupported`, keeps the first native |
There was a problem hiding this comment.
[Suggestion] Input-presentation ownership scope contradicts itself: line 121 defines it as sessionId + owner.id, but the Concurrent questions section (line 336) defines it as sessionId + owner.id + runId. An implementer following line 121 would key the map without runId, causing concurrent runs from the same owner in the same session to collide.
Concrete cost: a user asking two questions across parallel runs in the same chat session would see the second question fall back to text.
| Presentation ownership is scoped by `sessionId + owner.id`. Different users or | |
| sessions may have live input presentations simultaneously. Within one run, a | |
| second request in the same scope returns `unsupported`, keeps the first native | |
| Presentation ownership is scoped by `sessionId + owner.id + runId`. Different users or | |
| sessions may have live input presentations simultaneously. Within one run, a | |
| second request in the same scope returns `unsupported`, keeps the first native |
中文说明
[Suggestion] 输入展示的归属范围存在矛盾：第 121 行定义为 sessionId + owner.id，但并发问题章节（第 336 行）定义为 sessionId + owner.id + runId。如果实现者按第 121 行来建立映射（不含 runId），同一会话中同一用户的并发 run 会冲突，导致第二个问题回退为文本。
— qwen3.7-max via Qwen Code /review
| const outgoingText = await this.prepareOutgoingText(text); | ||
| if ( | ||
| await this.interactionPresenter.closeOutput( | ||
| segment.segmentId, | ||
| outgoingText, | ||
| 'completed', | ||
| segment, | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] When closeOutput returns false (e.g. run already terminal), the fallback path at line 1387 calls sendResponseMessage(chatId, text, sessionId) with the original unprepared text. This routes through sendReply then prepareOutgoingText(text), which uploads every image a second time.
Concrete cost: each image in the response triggers an extra DingTalk media upload API call on every affected response.
Fix: pass the already-prepared outgoingText to the fallback instead of the raw text, or move prepareOutgoingText after the closeOutput check.
中文说明
[Suggestion] 当 closeOutput 返回 false 时（例如 run 已终止），fallback 路径用原始未处理的 text 调用 sendResponseMessage，导致每张图片被上传两次。修复：将已处理好的 outgoingText 传给 fallback，或将 prepareOutgoingText 移到 closeOutput 判断之后。
— qwen3.7-max via Qwen Code /review
| execute: async () => { | ||
| const cancelled = await this.options.cancelRun( | ||
| record.sessionId, | ||
| record.runId, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] execute does not catch errors from this.options.cancelRun(). If cancelRun throws (e.g. transient network error), the error propagates out but record.stopClaimed stays true. Every subsequent Stop click on the same card returns { kind: 'ignored' }, making the Stop button permanently non-retryable.
Concrete cost: a single transient failure during cancel makes the Stop button useless for the rest of the card's lifetime.
Fix: wrap the body in try/catch and reset stopClaimed = false when the record is still current and non-terminal.
中文说明
[Suggestion] execute 没有捕获 this.options.cancelRun() 的异常。如果 cancelRun 抛出错误，record.stopClaimed 保持为 true，后续每次点击停止都会返回 ignored，导致停止按钮永久不可重试。修复：用 try/catch 包裹，在 record 仍然有效且未终止时重置 stopClaimed = false。
— qwen3.7-max via Qwen Code /review
| const cardRunId = this.cardRunBySession.get(sessionId); | ||
| if (cardRunId) { | ||
| this.cardRunBySession.delete(sessionId); | ||
| this.interactionPresenter?.terminalizeRun(cardRunId, 'cancelled'); | ||
| this.cardRuns.delete(cardRunId); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The new onSessionDied card-run cleanup is not covered by any test. If a session dies while a question card is pending, this code should terminalize the card run as cancelled and clean up both maps. A regression here would leave orphan card records and a live question card that appears interactive but whose underlying permission is already gone.
Suggested test: seed a card run via cardRunBySession and cardRuns, call onSessionDied, assert terminalizeRun was called with the matching run ID and cancelled, and both maps are cleaned up.
中文说明
[Suggestion] 新增的 onSessionDied 卡片运行清理逻辑没有测试覆盖。如果会话终止时有待处理的问题卡片，这段代码应该将卡片 run 终止为 cancelled 并清理两个 map。建议添加测试覆盖此路径。
— qwen3.7-max via Qwen Code /review
BZ-D
left a comment
BZ-D
left a comment
There was a problem hiding this comment.
复审 head 4c8458e7e633099bd7957f65e9d5940c2f39156d：上轮 Critical 已核实为当前 ChannelBase 串行调度模型下不可达，原线程已记录并关闭。新增的终态卡片内容脱敏逻辑及其 response/input/completed/failed/cancel 覆盖通过审查，未发现新的 Critical 或 Suggestion。\n\n验证：\n- packages/channels/dingtalk: 51/51（interaction presenter、status/question card controller）\n- packages/channels/base: 550/550，且 package TypeScript 检查通过\n- git diff --check 通过。
Maintainer verification — local build, mutation matrix, and wire-level E2E at head
|
| package | suite | result | typecheck |
|---|---|---|---|
channels/base |
vitest, 19 files | ✅ 955/955 | ✅ tsc --build |
channels/dingtalk |
vitest, 10 files | ✅ 248/248 | ✅ tsc --build |
channels/feishu |
vitest, 3 files | ✅ 110/110 | ✅ tsc --build |
channels/wecom |
vitest, 1 file | ✅ 134/134 | ✅ tsc --build |
sdk-typescript |
vitest test/unit, 29 files |
✅ 1430/1430 | ✅ tsc --noEmit |
2,877 tests, 0 failures. I enumerated the changed test files from the head itself rather than the self-report, so this includes the 7 dingtalk tests added after the author's "241 DingTalk tests" comment.
2 · Local merge with today's main
git merge origin/main is clean — no conflicts (merge 07d27cdba2 = 4c8458e7e6 + a7b1150816), git diff --check clean. All five suites pass on the merged tree (base 956/956 and sdk 1432/1432 — the extra tests are main's own additions). The wire E2E below also passes identically on the merged tree.
3 · Mutation matrix — the shipped tests really pin the invariants
Six hand-applied mutants against the load-bearing safety properties; suites re-run after each, worktree restored to pristine head between rounds. 6/6 killed, each by a precise single-invariant test failure (the rest of each suite stays green):
| # | invariant attacked | killed by |
|---|---|---|
| M1 | exact-run cancellation (ChannelBase.requestPromptRunCancellation drops the active.runId !== runId guard) |
base cancels only the current exact run identity |
| M2 | owner-only question submission | dingtalk claims one owner callback and submits validated answers |
| M3 | one forbidden notice per actor/card (dedup removed) | same discriminating test — it asserts the forbidden→ignored sequence |
| M4 | terminal image-path sanitization removed (the ae81e858 fix) |
4 × dingtalk hides local image paths when output ends with … (all four end reasons) |
| M5 | same-run second question no longer falls back | dingtalk keeps the first card active when the same run requests another question |
| M6 | DaemonSessionClient.cancel() coalescing removed |
sdk coalesces a prompt abort with an explicit session cancel |
4 · Wire-level E2E over real HTTP
Since the unit suites mock the card client, I also drove the real DingtalkInteractionPresenter + StatusCardController + QuestionCardController + DingtalkInteractiveCardClient chain with real timers (500 ms flush coalescing, 1.5 s question timeout) and real fetch over loopback HTTP. Only api.dingtalk.com is replaced by a local recording server via the injectable fetch — paths, methods, headers and bodies are the client's own. 27/27 checks passed, identically at the head and on the merged tree. Highlights from the captured request log:
- One status card per segment,
callbackType=STREAM, in-place full-snapshot streaming updates;[IMAGE: /Users/…]markers were sanitized to[Image pending]in every streamed and terminal payload — no local path ever reached the wire (content,copy_content,blockListall checked). - Non-owner Stop →
forbiddenonce, then silent on repeat; owner Stop → exactly one daemon cancellation for(sess-1, run-1), card terminalizedStoppedwith the button removed; replaying the same callback afterwards is ignored. - Question card renders single-select + multi-select + Other; unknown form keys and unlisted option values are rejected whole; the owner's submit delivered structured answers to the original request:
{"approach":"Use a feature flag","scope":"core, cli"}(Other free-text + joined multi-select), card updated in place to Submitted, duplicate replay ignored. - Same-run second question → text fallback while the first card stays authoritative; steer supersession → old card
Expiredwith zero synthetic answers to the old request; unanswered card expired by a real timer with a cancelled outcome; API 500 on create →handled+ text fallback, no blank running card.
5 · One maintenance note (not a blocker)
My first E2E draft drove the controllers without modeling run terminalization, and in that state the superseded question card of an older run remains answerable — exactly the scenario of the closed Critical thread. Under the real lifecycle (per-session serial dispatch; the adapter terminalizes the old run before the replacement starts) the old card is expired with no synthetic answer, which S4 confirms. In other words, the closed thread's unreachability rationale is what carries this safety property — worth remembering if the per-session serial dispatch model ever changes.
Not covered here
Real-device DingTalk rendering/callbacks (the author's GIFs and log appendices cover that), Windows, and the restart-safety / multi-worker limits the PR already declares out of scope.
中文版本
维护者验证 — 在 head 4c8458e7e6 上本地构建、变异矩阵与线级 E2E
结论:就本轮验证而言可以合并。 在精确 head 上全新隔离构建,受影响的测试套件全绿,针对本 PR 安全不变量的变异矩阵 6/6 击杀,基于真实 HTTP 的 27 项线级 E2E 全部通过,与今日 main 的本地合并干净且复测全绿。
环境
- 在 head
4c8458e7e633099bd7957f65e9d5940c2f39156d(与最近一轮 APPROVED 评审相同)建立独立 scratch worktree,独立npm ci+ 完整 workspace 构建 — Node v22.23.1、macOS,不复用共享检出。 - merge-base 为
0bafd0db21(fix(core): auto-retry transient network errors during API calls #7898)。main此后前进了 35 个提交,涉及packages/channels/base(DaemonChannelBridge)和 SDK daemon client,因此以下全部结论也在本地合并树上复测。
1 · head 上的完整套件与类型检查
channels/base 955/955、channels/dingtalk 248/248、channels/feishu 110/110、channels/wecom 134/134、sdk-typescript unit 1430/1430,五个包 typecheck 全部通过;共 2,877 个测试 0 失败。变更测试文件按 head 自行枚举而非采信自述,因此包含作者"241 个钉钉测试"评论之后新增的 7 个测试。
2 · 与今日 main 的本地合并
git merge origin/main 无冲突(合并提交 07d27cdba2 = 4c8458e7e6 + a7b1150816),git diff --check 干净。合并树上五个套件全绿(base 956、sdk 1432,多出的是 main 自身新增测试);下述线级 E2E 在合并树上结果一致。
3 · 变异矩阵 — 随 PR 提交的测试确实钉住了不变量
针对承载安全性的六处代码各施加一个变异,逐轮重跑套件并还原源码。6/6 击杀,且每次都是单一判别测试精确失败(套件其余部分保持绿色):M1 精确 run 取消(去掉 active.runId !== runId 守卫)、M2 仅 owner 可提交、M3 每 actor/card 仅一次无权限提示、M4 终态卡片本地图片路径脱敏(即 ae81e858 修复,4 个终态路径测试全部失败)、M5 同 run 第二个问题降级、M6 SDK 取消合并。
4 · 真实 HTTP 的线级 E2E
单元测试对卡片 client 使用 mock,因此另以真实 DingtalkInteractionPresenter + 两个卡片控制器 + DingtalkInteractiveCardClient 链路、真实定时器(500 ms 流式合并、1.5 s 提问超时)、真实 fetch 走回环 HTTP 驱动;仅通过可注入 fetch 把 api.dingtalk.com 换成本地记录服务器,路径、方法、请求头与请求体均为 client 原样产物。27/27 检查通过,head 与合并树结果一致。 要点:
- 每个输出段一张状态卡,
callbackType=STREAM,原地全量快照流式更新;[IMAGE: /Users/…]在所有流式与终态载荷中被替换为[Image pending],content、copy_content、blockList中均无本地路径外泄。 - 非 owner 点击 Stop → 首次
forbidden、重复静默;owner Stop → 对(sess-1, run-1)恰好一次 daemon 取消,卡片终态Stopped并移除按钮;之后重放同一回调被忽略。 - 提问卡渲染单选、多选与 Other;未知表单键与未列出的选项值整体拒绝;owner 提交后结构化答案送达原请求:
{"approach":"Use a feature flag","scope":"core, cli"}(Other 自由文本 + 多选拼接),卡片原地更新为 Submitted,重复提交被忽略。 - 同 run 第二个问题走文本降级、第一张卡保持权威;steer 取代 → 旧卡
Expired且对旧请求零合成答案;未回答卡片由真实定时器过期并返回 cancelled;建卡遇 API 500 →handled+ 文本降级,不留空白 Running 卡。
5 · 一条维护提示(非阻塞)
E2E 初版未建模 run 终止流程,此时旧 run 被取代后的提问卡仍可被回答 — 正是已关闭 Critical 线程描述的场景。在真实生命周期下(会话内串行调度;适配器在替换 run 启动前先终止旧 run),旧卡被置为过期且无合成答案,S4 已验证。换言之,该安全属性由已关闭线程论证的"不可达性"承载 — 若未来会话内串行调度模型发生变化,需要重新审视这一点。
本轮未覆盖
钉钉真机渲染与回调(作者的 GIF 与日志附录已覆盖)、Windows,以及 PR 已声明超出范围的重启恢复 / 多 worker 限制。
|
@qwen-code /triage |
Local E2E verification — DingTalk interactive cardsI verified this PR locally by running the real Verdict: the behaviour claimed in the Test Plan reproduces. 22 scenarios / 87 assertions pass on the current head
raw text (if the image has not resolved yet)How it was driven
Assertions read the captured HTTP request bodies, and the screenshots below are rendered by replaying those exact bodies — no hand-drawn mock-ups. 1. One status card, updated in place
raw capture2. Stop is bound to one exact runOwner presses Stop mid-stream → the upstream model request is aborted, the card becomes
3. Structured question card round-tripThe killer observable: the continuation card contains the model echoing its own tool result, so the answer is confirmed to have reached the agent's context — not just the card UI.
Also confirmed: the card updates in place to 4. Owner binding and multi-user isolationNon-owner submit → exactly one group notice per actor/card, card stays
5. Before / after (merge-base A/B)Same prompt, same
I also ran the identical flow on this PR with cards switched off: byte-for-byte the same text path as merge-base, so the fallback is a genuine no-op for existing deployments. 6. Degradation and config gates
Observations (non-blocking)
Two small notes for the docs: Alongside
中文版（合并参考）本地端到端验证 —— 钉钉交互卡片我在本地用真实的 结论：Test Plan 中声明的行为均可复现。在当前 head 验证要点
观察（不阻塞合并）
另有两处适合补进文档： 同时跑过： |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
The DingTalk interactive cards change (QwenLM#6930) added concurrent session-cancellation coalescing to DaemonSessionClient, growing the minified browser daemon bundle to 180295 bytes — 71 bytes over the 176KB budget, breaking npm run build on main. Bump the budget to 177KB following the established pattern. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Released in v0.21.2. |
6 participants
What this PR does
This PR adds opt-in DingTalk interactive status and question cards behind a transport-neutral Channel interaction contract. The shared layer identifies each attended prompt with an opaque
runId, identifies each contiguous visible-output interval with asegmentId, and presents structured user-input requests with their original request, owner, session, run, and delivery target. Adapters that do not implement the optional presentation hook returnunsupportedand retain the existing text and permission fallback.Architecture
1. End-to-end runtime chain
2. Existing adapter compatibility and degradation
3. Future IM adapter extension boundary
user,thread,chat_thread, orsinglescope. The interaction contract is attached after routing and does not introduce a second session model.collect/steer/followupdispatch semantics, the active run, output-segment boundaries, pending permission settlement, and exact-run cancellation.Completed,Failed,Stopped, orCancelled.ask_user_questionfields, submits structured answers to the original request, keeps only the latest pending card valid in one owner/session scope, and keeps different owners and sessions independent.accepted,forbidden, orignored, acknowledges the DingTalk frame, and only then executes accepted work.forbiddenproduces one deduplicated IM-only notice per actor/card;ignoredremains silent. Neither enters Agent context or mutates the question card.The status and question lifecycles are deliberately independent. A question closes the current output segment with an
input_requestedboundary, but it does not terminate the run. After an answer is accepted, subsequent model output receives a new segment identity while remaining in the same run and session. If the same run asks again while its first question is still pending, the first native card remains authoritative and the second request uses the explicit text fallback instead of creating an invisible competing card. A question from a newer run in the same owner/session scope expires the older card without responding to the older Agent request.Compatibility and degradation
interactiveCardsis configured. With no configuration, DingTalk keeps the original Markdown and permission behavior.blockStreaming=ondisables only the streaming status-card projection; question cards remain independently eligible.unsupported, and all added output parameters are optional. Feishu, QQ, plugins, and future adapters do not need DingTalk concepts or callback APIs. No Feishu or WeCom production adapter is changed.Example opt-in configuration:
{ "interactiveCards": { "enabled": true, "statusCard": { "enabled": true }, "questionCard": { "enabled": true, "timeoutMs": 270000 } } }Why it's needed
DingTalk Markdown delivery cannot update one response in place, bind a historical Stop button to one exact prompt run, or return structured
ask_user_questionanswers. Session identity alone is insufficient because one session can execute consecutive runs and one run can contain output before and after a human question. The shared run, segment, request, owner, and target contract provides the correlation needed for safe IM interaction while keeping platform rendering and callback state inside the DingTalk adapter.The design preserves context and dispatch behavior instead of creating a DingTalk-only conversation model. A user message that supersedes an active
steerrun settles that run's pending question before the replacement prompt starts;collectandfollowupcontinue to buffer or queue through Channel Base. Different users can retain independent cards when their configured routing scopes resolve to different sessions or active owners.Reviewer Test Plan
How to verify
[IMAGE: /path]marker.Stopped, and a later prompt remains unaffected. Repeat a stale or duplicate callback and confirm it does not cancel the new run.ask_user_questionwith single-select, multi-select, and Other input. Confirm one question card returns structured answers to the original permission request, updates in place toSubmitted, and allows continuation output only after the terminal question projection.Expiredwithout sending a synthetic answer to the Agent.steer, send ordinary text while a question card is pending. Confirm the old run exits, its card becomes unavailable, and a stale callback cannot settle the replacement run. Repeat undercollectandfollowupand confirm the existing buffering/queueing semantics are preserved.Evidence (Before & After)
Before: DingTalk delivered assistant output as separate Markdown messages and relayed permissions through text commands. It had no in-place streamed response, exact-run Stop boundary, or structured answer form.
After: DingTalk projects the shared Channel identities into two independent card lifecycles while the Agent, session router, dispatch modes, and permission contract remain authoritative.
Streaming status card and exact-run Stop
Structured question-card submission
Terminal Stopped projection
At code commit
ecfa1937981693f163ec32ad56d4cfc1bc84cf58, GitHub's Ubuntu Node 22 full test job passed. Earlier focused verification passed 955 Channel Base tests, 241 DingTalk tests, 110 Feishu tests, 134 WeCom tests, and 348 TypeScript SDK daemon-client tests, together with workspace build, typecheck, bundle, no-AK integration smoke, and Web Shell E2E smoke. The current head334e748e90808cb606a761ac575676a6b48d0289only refreshes the design diagrams and their documentation references and triggers a fresh CI run.Real-device E2E covered DingTalk streaming, terminal content retention, owner Stop, structured answers, sequential questions, stale and repeated callbacks, non-owner isolation, a 270-second question-card timeout, and card-disabled degradation. Feishu real-device E2E covered complete, Stop, steer, quoted context, and thread reuse without a production adapter change.
Tested on
Environment (optional)
macOS, Node.js 22, the bundled CLI running a daemon-managed Channel worker on loopback, AliDing with the configured DingTalk robot, and the Feishu desktop client for compatibility E2E. The DingTalk real-device run used
sessionScope=thread,dispatchMode=steer, open sender/group/DM policies, and required a mention in groups; these are runtime configuration choices rather than card-architecture requirements.Risk & Scope
threadId, so thread-oriented scopes use the existing conversation fallback rather than claiming topic isolation. Feishu retains its existing generic permission fallback forask_user_question; this PR does not add a Feishu structured question card. Free-form answer parsing and cross-platform identity federation are also out of scope.Linked Issues
Related to #6443.
中文说明
本 PR 做了什么
本 PR 在一套与传输平台无关的 Channel 交互契约之上，为钉钉增加可选启用的流式状态卡和结构化提问卡。共享层为每次有人参与的 Prompt 执行分配不透明的
runId，为每段连续、用户可见的输出分配segmentId，并把结构化提问连同原始请求、owner、session、run 和发送目标交给适配器。未实现可选展示 hook 的适配器返回unsupported，继续使用原有文本和权限降级链路。架构
上方三张图分别说明：从消息路由到钉钉卡片的完整运行链路、现有适配器的兼容与降级边界、未来飞书/企业微信等 IM 的扩展方式。
user、thread、chat_thread或single选择上下文。交互契约挂在路由之后，不引入第二套会话模型。collect/steer/followup调度语义、当前 run、输出段边界、待处理权限结算和精确 run 取消。Completed、Failed、Stopped或Cancelled。ask_user_question字段，把结构化答案提交回原请求，在同一 owner/session scope 中只保留最新待回答卡，并隔离不同 owner 和 session。accepted、forbidden或ignored，先确认钉钉 frame，再执行合法操作。forbidden每个 actor/card 只产生一次 IM 侧提示，ignored保持静默；二者都不进入 Agent 上下文，也不修改提问卡。状态卡与提问卡是两套独立生命周期。提问会以
input_requested边界关闭当前输出段，但不会结束 run；回答被接受后，模型的后续输出在同一 run/session 中获得新的 segment。若同一个 run 在第一张卡仍待回答时再次提问，第一张原生卡继续有效，第二个请求走明确的文本降级，不产生不可见的竞争卡。若同一 owner/session scope 中出现较新 run 的提问卡，旧卡应变为Expired，但不能向旧 Agent 请求发送合成答案。兼容与降级
interactiveCards才启用交互卡；未配置时，钉钉保持原有 Markdown 和权限处理行为。blockStreaming=on只关闭流式状态卡投影，提问卡仍可独立启用。unsupported，所有新增输出参数均为可选。飞书、QQ、插件和未来适配器无需理解钉钉概念或回调 API；本 PR 不修改飞书或企业微信的生产适配器。可选启用配置示例：
{ "interactiveCards": { "enabled": true, "statusCard": { "enabled": true }, "questionCard": { "enabled": true, "timeoutMs": 270000 } } }为什么需要
钉钉 Markdown 无法原地更新同一条响应、无法把历史 Stop 按钮绑定到某一次精确 Prompt run，也无法把结构化
ask_user_question答案返回原始请求。只有 session 标识不够，因为一个 session 可以连续执行多个 run，而一个 run 也可能在人工提问前后产生多段输出。共享的 run、segment、request、owner 和 target 契约提供了安全 IM 交互所需的关联，同时把平台渲染和回调状态留在钉钉适配器内。该设计保留原有上下文和调度行为，不创建钉钉专属会话模型。用户消息在
steer下替换当前运行时，会先结算被替换 run 的待回答问题，再启动新 Prompt；collect与followup继续由 Channel Base 暂存或排队。若配置的路由 scope 把不同用户解析到不同 session 或 active owner，不同用户可以同时保留各自独立的卡片。评审验证计划
如何验证
[IMAGE: /path]标记。Stopped，后续 Prompt 不受影响；重复或过期回调不能取消新 run。ask_user_question。确认提问卡把结构化答案返回原权限请求、原地更新为Submitted，并且继续输出只会发生在提问终态投影之后。Expired，且不会给 Agent 注入合成答案。steer下，提问卡待回答时发送普通文本。确认旧 run 真正退出、旧卡失效、旧回调不能结算新 run；再以collect和followup验证原有暂存/排队语义不变。前后效果与证据
修改前：钉钉把助手输出作为独立 Markdown 消息发送，并通过文本命令转发权限请求，不支持原地流式响应、精确 run Stop 或结构化答案表单。
修改后：钉钉把共享 Channel 身份投影为两套独立卡片生命周期，同时 Agent、session 路由、dispatch 模式和权限契约继续保持权威。上方三段 GIF 分别展示流式状态卡与 Stop、结构化提问提交、最终 Stopped 状态。
在代码提交
ecfa1937981693f163ec32ad56d4cfc1bc84cf58上，GitHub Ubuntu Node 22 全量测试任务通过。此前的聚焦验证通过 955 个 Channel Base 测试、241 个钉钉测试、110 个飞书测试、134 个企业微信测试和 348 个 TypeScript SDK daemon-client 测试，并通过 workspace build、typecheck、bundle、无 AK integration smoke 与 Web Shell E2E smoke。当前 head334e748e90808cb606a761ac575676a6b48d0289只更新三张设计图及其文档引用，并触发新一轮 CI。真机 E2E 已覆盖钉钉流式输出、终态正文保留、owner Stop、结构化答案、连续提问、过期/重复回调、非 owner 隔离、270 秒提问卡超时和卡片关闭降级。飞书真机 E2E 覆盖完成、Stop、steer、引用上下文与 thread 复用，且没有修改飞书生产适配器。
验证环境
macOS、Node.js 22、由 daemon 管理并运行在 loopback 上的 Channel worker、配置机器人后的阿里钉，以及用于兼容性 E2E 的飞书桌面客户端。钉钉真机运行采用
sessionScope=thread、dispatchMode=steer、开放 sender/group/DM policy，并要求群内 @；这些是运行时配置选择，不是卡片架构要求。Linux 由 GitHub Ubuntu Node 22 CI 验证；Windows 未单独真机验证。风险与范围
threadId，因此 thread 类 scope 使用已有会话回退，不能宣称话题隔离。飞书继续使用原有通用权限降级处理ask_user_question；本 PR 不增加飞书结构化提问卡。自由文本答案解析和跨平台身份联邦也不在范围内。关联 Issue
关联 #6443。