Skip to content

feat(daemon): add explicit channel delivery - #7388

Merged
wenshao merged 39 commits into
QwenLM:mainfrom
BenGuanRan:feat/channel-delivery-v1
Jul 23, 2026
Merged

feat(daemon): add explicit channel delivery#7388
wenshao merged 39 commits into
QwenLM:mainfrom
BenGuanRan:feat/channel-delivery-v1

Conversation

@BenGuanRan

Copy link
Copy Markdown
Collaborator

What this PR does

Adds one explicit Channel delivery contract for daemon notifications, Agent prompt finals, and scheduled-task finals. Authenticated callers select a named Channel plus a typed user or chat target; the daemon routes only to the matching worker owned by the resolved workspace, and adapters return sanitized permanent or retryable transport failures.

A synchronous notification endpoint reports success only after the adapter accepts the message. Prompt requests keep their asynchronous admission response and emit a correlated delivery-result event after a successful final answer. Scheduled tasks persist the same optional destination, run in their existing dedicated session, and deliver the completed final without starting another Agent turn.

Existing Channel webhooks keep their independent inbound execution path and asynchronous admission contract. Calls that omit delivery retain their current responses, events, session behavior, and scheduling semantics.

Why it's needed

Web alerts and daemon-driven Agent work need a production-visible way to reach a specific IM user or chat. The previous scheduled-only draft in #7153 coupled delivery to one producer and introduced a durable retry outbox before that behavior was required. A smaller shared boundary supports the immediate use cases while keeping execution failure separate from delivery failure and making stopped workers, invalid targets, authentication errors, throttling, and platform failures explicit.

Reviewer Test Plan

How to verify

Start a daemon with a configured Channel worker and mutation authentication. Verify that a direct notification returns success only after provider acceptance; an unavailable worker or invalid target returns a sanitized explicit error. Submit a prompt with delivery and confirm the request remains an asynchronous admission response, the Agent emits its normal completion event, and a correlated delivery-result event follows. Create a one-shot scheduled task with the same destination and confirm it fires automatically, produces its final in the bound session, sends exactly once, and is removed after execution. Repeat prompt and scheduled calls without delivery and confirm their existing behavior is unchanged. Confirm inbound Channel webhooks still return their existing asynchronous admission response.

Evidence (Before & After)

Before: daemon clients could not use one explicit destination contract across immediate notifications, prompt finals, and scheduled finals; the open draft covered scheduled delivery only.

After: all three producers use the same workspace-owned worker and adapter boundary. A real DingTalk run connected through Stream, delivered a synchronous notification, delivered a prompt final after asynchronous admission and turn completion, and delivered an automatically fired one-shot scheduled final. Provider acceptance was observed for each message; temporary credentials and runtime artifacts were removed afterward.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Node.js 25 local development runtime, isolated daemon, real DingTalk application credentials and Stream connection. Build and workspace typecheck passed. Focused validation passed: CLI 1,656 tests; Core scheduler 166; Channel base 504; DingTalk 76; Feishu 73; ACP Bridge 482; TypeScript SDK 405.

Risk & Scope

  • Main risk or tradeoff: final-answer delivery spans session completion, daemon-to-worker IPC, and provider-specific proactive APIs; failures are surfaced separately from successful Agent execution, but this first version does not persist or retry failed sends.
  • Not validated / out of scope: human read receipts, contact discovery and target-selection UI, durable delivery retries, cross-workspace fallback, and standalone Channel start behavior. Windows and Linux were not exercised locally.
  • Breaking changes / migration notes: none. Delivery is optional, and existing calls without it are unchanged.

Linked Issues

Closes #7387

Supersedes #7153 and the scheduled-only scope in #7152.

中文说明

本 PR 做了什么

新增统一、显式的 Channel 投递契约,支持 daemon 通知、Agent Prompt 最终答案和定时任务最终答案。已鉴权调用方指定 Channel 名称及用户或群聊目标;daemon 只会路由到当前 workspace 所属的匹配 Worker,Adapter 会返回经过脱敏的永久性或可重试传输错误。

同步通知接口只有在 Adapter 接受消息后才返回成功。Prompt 请求保持异步受理响应,在成功产生最终答案后发布可关联的投递结果事件。定时任务持久化同一种可选目标,在现有独立 Session 中执行,并直接投递完成后的最终答案,不会再启动一次 Agent 执行。

现有 Channel Webhook 继续使用独立的入站执行路径和异步受理契约。未传 delivery 的调用保持原有响应、事件、Session 行为和定时语义不变。

为什么需要

Web 告警和 daemon 驱动的 Agent 任务需要一种生产可见的能力,将结果发送到指定 IM 用户或群聊。旧的 #7153 仅覆盖定时任务,并在当前需求尚未要求时引入持久化重试信箱。更小的统一底座可以覆盖即时场景,同时将执行失败与投递失败分离,并明确暴露 Worker 未启动、目标非法、鉴权失败、限流及平台故障。

Reviewer 测试计划

如何验证

使用已配置 Channel Worker 和写操作鉴权的 daemon 启动服务。验证直接通知只有在平台接受后返回成功,Worker 不可用或目标非法时返回明确且脱敏的错误。提交带 delivery 的 Prompt,确认接口仍为异步受理,Agent 正常产生完成事件,随后发布可关联的投递结果事件。创建带相同目标的一次性定时任务,确认由调度器自动触发,在绑定 Session 中产生最终答案,只发送一次并在执行后移除。再验证不带 delivery 的 Prompt 和定时任务行为保持不变,并确认入站 Channel Webhook 仍保持原有异步受理响应。

前后对比证据

之前:daemon 客户端无法通过统一的显式目标契约覆盖即时通知、Prompt 最终答案和定时任务最终答案;现有 Draft 只支持定时投递。

之后:三类生产者共用同一套 workspace 所属 Worker 和 Adapter 底座。真实钉钉测试通过 Stream 建连,成功投递同步通知、在 Prompt 异步受理及执行完成后投递最终答案,并成功投递由调度器自动触发的一次性定时任务最终答案。三条消息均获得平台接受;测试后已清理临时凭证和运行产物。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境

Node.js 25 本地开发运行时、隔离 daemon、真实钉钉应用凭证及 Stream 连接。构建及 workspace typecheck 通过。重点验证通过:CLI 1,656 条、Core 调度 166 条、Channel Base 504 条、DingTalk 76 条、Feishu 73 条、ACP Bridge 482 条、TypeScript SDK 405 条。

风险与范围

  • 主要风险或取舍:最终答案投递跨越 Session 完成、daemon 到 Worker IPC 及平台主动消息 API;投递失败与成功的 Agent 执行分开呈现,但首版不持久化或重试失败消息。
  • 未验证或不在范围内:人工已读回执、联系人发现及目标选择 UI、持久化投递重试、跨 workspace 回退、独立 Channel start 行为。Windows 和 Linux 未在本地执行。
  • 破坏性变更或迁移说明:无。delivery 为可选字段,未传入时现有调用保持不变。

关联 Issue

Closes #7387

替代 #7153 以及 #7152 中仅限定时任务的范围。

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Real IM E2E report

Tested commit: 106592a861d11d65268869262bf303c167512a08

Environment: isolated local daemon on macOS, real Qwen DingTalk application credentials, authenticated mutation routes, DingTalk Stream connection, one previously observed direct-message target. Credentials and target identifiers are intentionally omitted.

Scenario Result
Synchronous notification POST /workspace/notify returned 200, delivered: true, and a delivery ID after provider acceptance.
Prompt final Prompt admission returned 202; the session emitted turn_complete, followed by a correlated channel_delivery_result with source: prompt and status: delivered. The delivered text exactly matched the Agent final.
Scheduled final A one-shot task was scheduled for the next minute and allowed to fire through the scheduler; no manual run endpoint was used. The bound session final matched exactly, the task was consumed, and the delivery event reported source: scheduled and status: delivered.
Failure visibility A disabled/incorrect DingTalk application was rejected by the provider and surfaced as a sanitized permanent delivery error instead of a false success.

delivered means the IM provider accepted the message; this test does not claim a human read receipt. The isolated daemon was stopped after testing, all temporary settings, transcripts, task data, and logs were removed, and the feature worktree remained clean.

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: this is a feature addition backed by issue #7387 with clear acceptance criteria, labeled priority/P2, type/feature-request, daemon, and roadmap/background-automation. The need is concrete — daemon clients (web alerts, Agent results) have no production-visible way to reach a specific IM user or chat. The prior drafts (#7152, #7153) coupled delivery to one producer and introduced a durable retry outbox prematurely. This is an observed gap, not theoretical hardening.

Direction: aligned. Channel delivery for daemon notifications, prompt finals, and scheduled finals sits squarely on the roadmap/background-automation path. The unified contract across three producers is the right scope — it avoids duplicating platform routing while keeping execution semantics separate.

Size: this PR touches core paths (packages/core/src/services/cronScheduler.ts, cronTasksFile.ts, index.ts) and spans five packages (core, cli, channels, acp-bridge, sdk-typescript). Production logic lines (excluding tests and docs): ~2,182. Test lines: ~3,991. Docs: ~623. The core-specific production change is small (~110 lines — adding an optional delivery field to CronTaskDelivery and threading it through the scheduler), but the cross-package breadth triggers maintainer awareness. Flagging for @wenshao @tanzhenxin @yiliang114 @LaZzyMan — this is a large feat (2,182+ production lines, 64 files) that adds a new daemon delivery contract. The 1,000+ line advisory also applies: consider whether the SDK surface, docs, and implementation plan could land in a follow-up if review bandwidth is tight.

Approach: the scope feels right for the stated goal. The three producers (notify, prompt, scheduled) genuinely share one worker/adapter boundary, and the PR explicitly avoids the premature durable retry outbox from #7153. The design follows existing patterns — the delivery IPC mirrors the webhook IPC contract, the worker supervisor correlation mirrors webhook task settlement, and the ChannelBase.deliverProactive() extension is a natural fit. Every change in the diff appears necessary for the stated goal; no drive-by refactors or unrelated churn.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:这是一个有 issue(#7387)支撑的功能新增,带有明确的验收标准,标记为 priority/P2type/feature-requestdaemonroadmap/background-automation。需求是具体的——daemon 客户端(Web 告警、Agent 结果)目前没有生产可见的方式将内容发送到指定 IM 用户或群聊。之前的草案(#7152#7153)将投递耦合到单一生产者,并过早引入了持久化重试信箱。这是一个已观测到的缺口,而非理论性加固。

方向:对齐。Channel 投递覆盖 daemon 通知、Prompt 最终答案和定时任务最终答案,完全在 roadmap/background-automation 路径上。三个生产者共用统一契约的范围是正确的——避免重复平台路由,同时保持各自的执行语义。

规模:本 PR 触及核心路径(packages/core/src/services/cronScheduler.tscronTasksFile.tsindex.ts),跨越五个包(core、cli、channels、acp-bridge、sdk-typescript)。生产逻辑行数(不含测试和文档):约 2,182 行。测试行数:约 3,991 行。文档:约 623 行。核心特定的生产改动很小(约 110 行——添加可选的 delivery 字段并在调度器中传递),但跨包广度触发维护者关注。提醒 @wenshao @tanzhenxin @yiliang114 @LaZzyMan——这是一个大型 feat(2,182+ 生产行,64 个文件),新增 daemon 投递契约。1,000+ 行大 PR 建议也适用。

方案:范围对于既定目标是合理的。三个生产者(通知、Prompt、定时)确实共用一套 Worker/Adapter 底座,PR 明确避免了 #7153 中过早的持久化重试信箱。设计遵循现有模式。diff 中的每项改动对于既定目标都是必要的;没有顺手重构或无关改动。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 8b3f5f8a91ac525d1903f8e8801805df25047347 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 8b3f5f8, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

capabilities

field PR base (before) this PR (after)
features[] "channel_delivery"

Qwen Code · serve A/B

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: For this feature, I would add a deliverProactive() method to ChannelBase with typed target validation, mirror the existing webhook IPC contract for delivery requests/results, extend the worker supervisor with delivery correlation and timeout, add a synchronous POST /workspace/notify route, thread optional delivery metadata through prompt context and scheduled task persistence, and publish a sanitized channel_delivery_result event. The PR's approach matches this closely — it follows the established webhook IPC pattern for worker communication, which is the right call.

Findings — no critical blockers.

The implementation is well-structured and follows project conventions:

  • Security: the daemon prompt route strips caller-supplied delivery and _meta['qwen.daemon.channelDelivery'] before injecting trusted context through BridgeClientRequestContext.channelDelivery (bridge.ts:5356). The createBoundChannelDeliveryHandler validates authorization via ChannelDeliveryAuthorizationStore.consume() before dispatch — prompt deliveries are one-shot (deleted after consumption), scheduled deliveries are replay-protected via lastConsumedAt. No child-supplied workspace data is trusted.
  • Privacy: delivery result events never include message text, target IDs, or credentials. channelDeliveryPublicError() maps error codes to fixed strings — no internal details leak to callers. Diagnostic logging sanitizes worker errors through sanitizeWorkerDiagnostic and redacts target IDs.
  • Error handling: proper permanent/transient classification in DingTalk and Feishu adapters, sanitized error messages, 30-second IPC timeout, queue-full at 16 concurrent deliveries (MAX_CHANNEL_DELIVERIES_IN_FLIGHT), and graceful shutdown drain.
  • Backward compatibility: delivery is optional everywhere. Existing calls without delivery are unchanged. The missed-one-shot batch explicitly clears delivery so enabling Channel later cannot create a burst of old alerts.
  • Final-only delivery: commitChannelDeliveryResponseBlock only captures text when there are no function calls, and the checkpoint/rollback on RETRY/MODEL_FALLBACK ensures only the final successful response is delivered.
  • Conventions: ESM, strict TypeScript, kebab-case filenames, no any, no cross-package relative imports. Tests are collocated with source. The design doc and implementation plan are committed under docs/.
sequenceDiagram
    participant P1 as Caller
    participant P2 as Daemon Route
    participant P3 as BridgeClient
    participant P4 as Session
    participant P5 as Worker Supervisor
    participant P6 as Channel Worker
    participant P7 as ChannelBase Adapter

    P1->>P2: POST /session/:id/prompt (with delivery)
    P2->>P2: parse and validate delivery, strip from ACP payload
    P2->>P3: sendPrompt with trusted channelDelivery context
    P3->>P4: inject delivery metadata into _meta
    P4->>P4: collect final text on end_turn (skip if tool calls)
    P4->>P3: extMethod qwen/control/channel-delivery
    P3->>P3: validate session, correlation, bounds
    P3->>P5: onChannelDelivery handler (bound workspace)
    P5->>P5: authorization store consume (one-shot or replay-protected)
    P5->>P6: IPC channel_delivery message
    P6->>P7: deliverProactive(target, text)
    P7-->>P6: success or ChannelProactiveDeliveryError
    P6-->>P5: IPC channel_delivery_result
    P5-->>P3: delivered or failed
    P3->>P3: publish sanitized channel_delivery_result event
    P3-->>P4: result
Loading
Files changed (30 of 64 shown)
File What changed
docs/design/channel-delivery-v1.md Design doc defining the delivery contract and privacy rules
docs/developers/qwen-serve-protocol.md Protocol docs updated with Notify route and delivery event
docs/plans/2026-07-21-channel-delivery-review-fixes.md Implementation plan for review-fix rounds
docs/plans/2026-07-22-channel-delivery-final-only.md Plan for final-only delivery scoping
docs/superpowers/plans/2026-07-21-channel-delivery-v1.md Original 7-task implementation plan
packages/channels/base/src/ChannelBase.ts Added deliverProactive() with target validation and adapter dispatch
packages/channels/base/src/ChannelProactiveDeliveryError.ts New typed error class with permanent/transient disposition
packages/channels/base/src/types.ts Added ChannelProactiveTarget interface
packages/channels/dingtalk/src/DingtalkAdapter.ts Delivery target support, typed error classification
packages/channels/feishu/src/FeishuAdapter.ts Delivery via open_id for user targets, typed error classification
packages/acp-bridge/src/bridgeClient.ts Reverse extMethod handler with validation, correlation, sanitized event
packages/acp-bridge/src/bridgeOptions.ts ChannelDeliveryHandler type and BridgeOptions.onChannelDelivery
packages/acp-bridge/src/bridgeTypes.ts Trusted channelDelivery context and meta key
packages/acp-bridge/src/bridge.ts Strips spoofed delivery metadata, injects trusted context
packages/cli/src/serve/channel-delivery-authorization.ts Authorization store with one-shot and replay-protected consumption
packages/cli/src/serve/channel-delivery-ipc.ts IPC message types, validators, and ChannelDeliveryError
packages/cli/src/serve/channel-delivery.ts Public delivery parser and normalizer with text bounding
packages/cli/src/serve/channel-worker-supervisor.ts Delivery correlation, timeout, and pending-request rejection on exit
packages/cli/src/serve/channel-worker-group.ts Exact-workspace delivery routing without fallback
packages/cli/src/serve/channel-worker-manager.ts Manager-level delivery pass-through with drain guard
packages/cli/src/commands/channel/daemon-worker.ts Worker-side delivery execution with concurrency cap
packages/cli/src/serve/routes/channel-notify.ts Synchronous Notify routes with strict mutation auth
packages/cli/src/serve/routes/session.ts Prompt delivery parsing and trusted context injection
packages/cli/src/serve/routes/scheduled-tasks.ts Optional delivery persistence on task creation and PATCH
packages/cli/src/acp-integration/session/Session.ts Per-turn delivery collector, final-answer gating, retry rollback
packages/cli/src/serve/run-qwen-serve.ts createBoundChannelDeliveryHandler wired to all three bridge constructors
packages/core/src/services/cronTasksFile.ts CronTaskDelivery type, validation, and round-trip persistence
packages/core/src/services/cronScheduler.ts Delivery threading through durable task lifecycle
packages/sdk-typescript/src/daemon/DaemonClient.ts notify() on primary and workspace clients
packages/sdk-typescript/src/daemon/events.ts channel_delivery_result event schema and validator

Real-Scenario Testing

This feature requires a running daemon with configured Channel workers and real IM credentials (DingTalk/Feishu Stream connection) for full delivery testing. Without these external dependencies, end-to-end message delivery cannot be exercised in this environment. The PR author reports real DingTalk E2E verification with provider acceptance observed for all three producers.

Daemon smoke test (PR head 8b3f5f8a, built from source):

Started the daemon from the PR build and verified the delivery surface:

$ QWEN_SANDBOX=false node dist/cli.js serve --port 18924

=== CAPABILITIES (channel_delivery advertised) ===
"features": [
    ...
    "channel_delivery",
    "channel_control",
    ...
]

=== NOTIFY (no auth → 401) ===
$ curl -s -X POST http://localhost:18924/workspace/notify \
    -H 'Content-Type: application/json' \
    -d '{"channel":"test","target":{"type":"user","id":"u1"},"text":"hello"}'
{"error":"This route requires the daemon to be configured with a bearer token. Set QWEN_SERVER_TOKEN or pass --token to enable bearer auth.","code":"token_required"}
HTTP_CODE:401

=== NOTIFY (with auth, no worker → sanitized 400) ===
$ curl -s -X POST http://localhost:18924/workspace/notify \
    -H "Authorization: Bearer test-token-123" \
    -H 'Content-Type: application/json' \
    -d '{"channel":"test","target":{"type":"user","id":"u1"},"text":"hello"}'
{"error":"Invalid channel notification.","code":"channel_delivery_invalid"}
HTTP_CODE:400

=== NOTIFY (invalid body → sanitized 400) ===
$ curl -s -X POST http://localhost:18924/workspace/notify \
    -H "Authorization: Bearer test-token-123" \
    -H 'Content-Type: application/json' \
    -d '{"bad":"data"}'
{"error":"Invalid channel notification.","code":"channel_delivery_invalid"}
HTTP_CODE:400

The daemon starts cleanly, advertises channel_delivery in capabilities, the notify route enforces bearer auth (401 without token), and returns sanitized validation errors (no internal paths or stack traces leak).

Unit test verification (all passed on PR head 8b3f5f8a):

packages/core        cronTasksFile + cronScheduler     170 passed
packages/channels    ChannelBase                       506 passed
packages/channels    DingtalkAdapter                    84 passed
packages/channels    Feishu adapter                     75 passed
packages/acp-bridge  bridgeClient + bridge              514 passed
packages/cli         delivery (ipc, auth, supervisor,
                     group, manager, notify, serve)     410 passed
packages/cli         Session + daemon-worker +
                     scheduled-tasks + server         1,336 passed
packages/sdk         DaemonClient + events + surface   414 passed
─────────────────────────────────────────────────────────────────
Total                                              3,509 passed

TypeScript typecheck (tsc --noEmit) passed for packages/core. Full build (npm run build) passed. CI green at 8b3f5f8a (Test ubuntu, Serve A/B, web-shell E2E, Real daemon E2E, review-pr).

中文说明

代码审查

独立方案: 对于此功能,我会在 ChannelBase 上添加带类型化目标验证的 deliverProactive() 方法,镜像现有 Webhook IPC 契约用于投递请求/结果,扩展 Worker Supervisor 以支持投递关联和超时,添加同步 POST /workspace/notify 路由,将可选投递元数据通过 Prompt 上下文和定时任务持久化传递,并发布脱敏的 channel_delivery_result 事件。PR 的方案与此高度一致。

发现——无关键阻塞项。

实现结构良好,遵循项目规范:

  • 安全性: daemon Prompt 路由在注入受信任上下文之前剥离调用方提供的 delivery 元数据(bridge.ts:5356)。createBoundChannelDeliveryHandler 在分发前通过 ChannelDeliveryAuthorizationStore.consume() 验证授权——Prompt 投递为一次性(消费后删除),定时投递通过 lastConsumedAt 防重放。不信任子进程提供的工作区数据。
  • 隐私: 投递结果事件不包含消息文本、目标 ID 或凭证。channelDeliveryPublicError() 将错误码映射为固定字符串——不向调用方泄露内部细节。诊断日志通过 sanitizeWorkerDiagnostic 脱敏并遮蔽目标 ID。
  • 错误处理: DingTalk 和 Feishu 适配器中正确的永久/瞬态分类,脱敏错误消息,30 秒 IPC 超时,16 个并发投递队列满,以及优雅关闭排空。
  • 向后兼容: 投递在所有地方都是可选的。错过的一次性任务批次显式清除投递。
  • 仅最终答案投递: commitChannelDeliveryResponseBlock 仅在没有工具调用时捕获文本,RETRY/MODEL_FALLBACK 时的检查点/回滚确保只投递最终成功响应。
  • 规范: ESM、严格 TypeScript、kebab-case 文件名、无 any、无跨包相对导入。

真实场景测试

此功能需要运行中的 daemon、已配置的 Channel Worker 和真实 IM 凭证。没有这些外部依赖,在此环境中无法进行端到端消息投递测试。PR 作者报告了真实钉钉 E2E 验证。

Daemon 冒烟测试: 从 PR 构建启动 daemon,验证 channel_delivery 能力已广播,notify 路由强制 bearer 认证(无 token 返回 401),返回脱敏验证错误(无内部路径或堆栈泄露)。

单元测试验证: 共 3,509 个测试通过,覆盖所有变更文件。TypeScript 类型检查通过。完整构建通过。CI 全绿。

Qwen Code · qwen3.8-max-preview

Reviewed at 8b3f5f8a91ac525d1903f8e8801805df25047347 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review across every stage, but the Stage 0 maintainer-awareness escalation for a large cross-package feat (2,182+ production lines, 5 packages, core paths touched) needs a human sign-off.

Going back to my independent proposal: the PR's approach matches it closely. The delivery IPC mirrors the webhook IPC contract, the worker supervisor correlation mirrors webhook task settlement, and ChannelBase.deliverProactive() is a natural extension. I didn't find a materially simpler path — the three producers genuinely share one worker/adapter boundary, and the PR correctly avoids the premature durable retry outbox from #7153.

The code is straightforward and well-organized. Security is handled properly — caller-supplied delivery metadata is stripped at the bridge boundary and only trusted context is injected. The authorization store enforces one-shot consumption for prompts and replay protection for scheduled tasks. Privacy is maintained — result events carry no text, target IDs, or credentials, and public errors are fixed strings. Error classification is correct — permanent vs transient, sanitized messages, proper HTTP status mapping. Backward compatibility is preserved — delivery is optional everywhere, and the missed-one-shot batch explicitly clears delivery. The final-only delivery gating (skip tool-call blocks, rollback on retry/fallback) is correctly implemented across all three prompt loops.

All 3,509 unit tests pass across every changed file. TypeScript typecheck passes. Full build passes. CI is green at 8b3f5f8a (Test ubuntu, Serve A/B, web-shell E2E, Real daemon E2E, review-pr). The daemon smoke test confirms the capability is advertised, the notify route enforces auth, and errors are sanitized. The PR author reports real DingTalk E2E verification with provider acceptance observed for all three producers.

Since the last triage at 106592a, ~30 commits have landed — mostly review feedback fixes (typed error wrapping, authorization store, final-only delivery scoping, diagnostic sanitization, cron one-shot recovery, error code consolidation). Each round tightened the implementation without changing the architectural direction. The delta is sound.

The reason for 3/5 rather than 4–5/5 is purely policy: this is a large cross-package feat that touches core paths (packages/core/src/services/), and the Stage 0 gate requires maintainer awareness for feats of this size touching core infrastructure. The code quality itself would support a higher score. Deferring to the maintainer for the architectural sign-off on the delivery contract and the cross-package scope.

中文说明

置信度:3/5 — 每个阶段审查均干净,但 Stage 0 维护者关注升级(大型跨包 feat,2,182+ 生产行,5 个包,触及核心路径)需要人工签核。

回到我的独立方案:PR 的方案与之高度一致。投递 IPC 镜像 Webhook IPC 契约,Worker Supervisor 关联镜像 Webhook 任务结算,ChannelBase.deliverProactive() 是自然扩展。我没有找到更简路径。

代码直接且组织良好。安全处理得当——调用方提供的投递元数据在 bridge 边界被剥离,只注入受信任上下文。授权存储强制 Prompt 一次性消费和定时任务防重放。隐私得到维护——结果事件不携带文本、目标 ID 或凭证,公开错误为固定字符串。错误分类正确。向后兼容得到保留。仅最终答案投递门控(跳过工具调用块,重试/回退时回滚)在所有三个 Prompt 循环中正确实现。

所有 3,509 个单元测试通过。TypeScript 类型检查通过。完整构建通过。CI 全绿。Daemon 冒烟测试确认能力已广播、notify 路由强制认证、错误已脱敏。

自上次在 106592a 的审查以来,约 30 个提交已合入——主要是评审反馈修复。每一轮都在不改变架构方向的情况下收紧了实现。增量是合理的。

3/5 而非 4–5/5 的原因纯粹是策略:这是一个大型跨包 feat,触及核心路径,Stage 0 门控要求维护者关注。代码质量本身支持更高评分。转交维护者对投递契约和跨包范围进行架构签核。

Qwen Code · qwen3.8-max-preview

Reviewed at 8b3f5f8a91ac525d1903f8e8801805df25047347 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @wenshao @tanzhenxin @yiliang114 @LaZzyMan — fresh full review at 8b3f5f8a (re-run triggered by @wenshao). The code review is clean: no critical blockers, 3,509 unit tests pass, typecheck and build pass, CI green, daemon smoke test confirms the delivery surface works correctly. Since the last triage at 106592a, ~30 review-feedback commits have tightened the implementation (authorization store, final-only delivery, diagnostic sanitization, cron recovery fix, error code consolidation) without changing the architectural direction. The Stage 0 gate still requires maintainer awareness for this large cross-package feat (2,182+ production lines, 64 files, 5 packages, core paths touched). Needs a human call on the architectural direction of the unified delivery contract and the cross-package scope. If the maintainer agrees with the direction, this is merge-ready from the code quality perspective.

Comment thread packages/acp-bridge/src/bridgeClient.ts
Comment thread packages/cli/src/commands/channel/daemon-worker.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/cli/src/commands/channel/daemon-worker.ts Outdated
Comment thread packages/cli/src/serve/channel-delivery.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.test.ts
Comment thread packages/cli/src/serve/channel-delivery.test.ts Outdated
@QwenLM QwenLM deleted a comment from qwen-code-dev-bot Jul 21, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🚫 Takeover not engaged: fork takeover requires the PR author to hold write access on this repository (author BenGuanRan currently: read). A maintainer can adopt the PR instead: snapshot the head into an in-repo branch, open a new PR (commit authorship is preserved), and take that over.

中文说明

🚫 未接管:fork 托管要求 PR 作者在本仓库持有 write 及以上权限(作者 BenGuanRan 当前为:read)。维护者可改用领养:将 head 快照为本仓库分支并另开 PR(commit 署名保留),再对新 PR 执行接管。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

— qwen3.7-max via Qwen Code /review

Comment thread packages/acp-bridge/src/bridgeClient.test.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts
@QwenLM QwenLM deleted a comment from qwen-code-dev-bot Jul 21, 2026
@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Jul 21, 2026

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

— qwen3.7-max via Qwen Code /review

Comment thread packages/channels/dingtalk/src/DingtalkAdapter.ts Outdated
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

…error

A proactive token fetch failure escaped sendProactiveChunk as a plain Error, bypassing ChannelProactiveDeliveryError classification. Wrap it as a transient typed error (preserving the cause) so downstream classification dispatches on it consistently. Also cover the scheduled-source branch of the bridge channel-delivery ext-method, which previously had no test coverage.
@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Review: feat(daemon): add explicit channel delivery

Reviewed the full 64-file diff against main. This is a well-structured feature — the trust boundary in particular is the strongest part of the PR — but I found four behavioral issues worth addressing and some scope that should probably be split out.

What the PR does

Adds one delivery contract ({kind:'channel', target:{channelName,type,id}}) shared by three producers — synchronous POST /workspace(s)/:workspace/notify, POST /session/:id/prompt finals, and scheduled-task finals — routed through the daemon's workspace-owned Channel worker via a new IPC path ending at ChannelBase.deliverProactive(). Prompt/scheduled finals travel back over a new reverse extMethod qwen/control/channel-delivery, are authorized against a daemon-owned store, and surface as a replayable channel_delivery_result SSE event. New channel_delivery capability, SDK notify() + event types.

What's good

  • Trust boundary is right. The daemon mints the deliveryId, pins the target, strips caller-supplied delivery and _meta[qwen.daemon.channelDelivery] in bridge.ts:5342-5366, and re-injects only from BridgeClientRequestContext. Consume-once for prompts and monotonic firedAt for schedules are the correct primitives, and bridge.test.ts locks the spoofing case in.
  • Every parser uses an explicit key allowlist (Object.keys(...).every(...)) rather than lenient parsing — channel-delivery.ts, cronTasksFile.isValidDelivery, bridgeClient.handleChannelDelivery, events.isChannelDeliveryResultData.
  • Failure isolation holds: delivery never converts turn_complete into turn_error, and the daemon log helper is non-throwing.
  • The synthetic missed-one-shot batch explicitly clears delivery (cronScheduler.ts:1033), so enabling channels later can't replay a burst of old alerts. Good instinct.
  • Test coverage is genuinely broad (~2.9k lines), including the notify status-code matrix, strict:true assertions on both routes, sanitization assertions in the bridgeClient tests, and the SDK↔bridge error-code cross-check.

Issues

1. delivery on a scheduled task that can't be session-bound is silently deadpackages/cli/src/serve/routes/scheduled-tasks.ts:479

POST /scheduled-tasks persists delivery and echoes it in the 201 view unconditionally, but boundSessionId is only set when a bridge exists (manageScheduledTaskSessions). When it's absent:

  • durableTaskToJob drops the field — cronScheduler.ts:1638 requires task.delivery && task.sessionId;
  • registerScheduledTask is skipped — scheduled-tasks.ts:530 requires task.sessionId.

So the task fires forever and never delivers, with no error and no channel_delivery_result. Same for PATCH adding delivery to an unbound task. Suggest rejecting with 400 channel_delivery_invalid when delivery is present and no session will be bound.

2. Provider error text reaches the replayable SSE event without target-ID redactionpackages/cli/src/serve/run-qwen-serve.ts:210 / :252

The redaction is applied exactly where the text is not published, and skipped where it is:

if (isChannelDeliveryError(err)) {
  return failed(err.code, err.message);          // err.message → SSE data.error, never redacted
}
return failed('channel_delivery_failed', 'Channel delivery failed.', err);  // redacted, but only into the log

The worker-side message is only sanitizeWorkerDiagnostic(..., {daemonToken, workerEnv}) (daemon-worker.ts:895), which strips tokens/env but not target IDs or provider bodies — DingTalk's sendProactiveChunk embeds up to 300 chars of the raw provider response, Feishu embeds the HTTP status. That string flows worker → supervisor → ChannelDeliveryError.messagedata.error. This contradicts the documented invariant in docs/design/channel-delivery-v1.md ("never include … target IDs") and the protocol doc. Apply the same replaceAll(info.target.id, …) (or a fixed per-code message) to the error returned in ChannelDeliveryHostResult.

3. target.type is silently ignored for adapters that don't override pushProactiveDeliverypackages/channels/base/src/ChannelBase.ts:703-748, :804-840

deliverProactive() maps typeisGroup, but the default pushProactiveDeliverypushProactivesendMessage(target.chatId) ignores isGroup entirely, and the default supportsProactiveDeliveryTarget returns true for both types. Only Feishu overrides pushProactiveDelivery; DingTalk happens to work because its own pushProactive branches on isGroup. For Telegram/WeCom a type:'user' delivery is attempted as a chat send — accepted by validation, wrong (or failing) at the provider. Either make the delivery hook opt-in per adapter, or have the base reject when the adapter hasn't declared user/chat support.

4. DingTalk can never produce channel_delivery_rejectedpackages/cli/src/commands/channel/daemon-worker.ts:1094-1107

classifyChannelDeliveryError only maps ChannelProactiveDeliveryError with disposition:'permanent'. DingTalk's pushProactive throws plain Errors for invalid recipients, flow control, and HTTP 4xx; the base wrapper relabels them 'transient'channel_delivery_failed (502). Since DingTalk is the platform in the Evidence section and "make invalid targets explicit" is a stated goal, an invalid DingTalk user id is currently indistinguishable from a transport blip. (Task 1 of the v1 plan has this checkbox unchecked.)


Scope

5. An unrelated core scheduler change is bundled in. armedDurableOneShots (cronScheduler.ts:266,515,548,835,885,904,935,1280,1416) is not in main and changes durable one-shot missed-detection semantics for all tasks, delivery or not. The motivation is delivery-adjacent (don't let a sub-second reload convert an armed one-shot into a synthetic "missed" confirmation and lose its real delivery), but it's a timing-sensitive change to a shared scheduler. Prefer a separate PR; at minimum, call it out in the Risk & Scope section, which currently doesn't mention it.

6. 425 lines of agent execution plans land in the repo. docs/plans/2026-07-21-…, docs/plans/2026-07-22-…, and a brand-new docs/superpowers/plans/ tree contain sub-skill directives, per-task checkboxes referencing this PR number, and process items like "Commit and deliver the fixes to the maintainer-selected branch without resolving or replying to review threads". These aren't project documentation. Recommend dropping them and keeping docs/design/channel-delivery-v1.md + the protocol doc. Note also that docs/plans/2026-07-21-… leaves "Run real IM E2E for notify, Prompt final, scheduled final, and provider rejection" unchecked, which reads as contradicting the PR body's Evidence section.

7. The POST /session/:id/prompt protocol section is rewritten (202 admission, prompt_absolute_deadline, removal of the "Stage 1 limitation" block). It looks like a correct fix for stale docs, but it's independent of channel delivery and adds noise to an already large diff.


Minor

  • Session.ts:2749,3493,4814channelDeliveryCheckpoint is always 0, because beginChannelDeliveryResponseBlock returns a fresh []. All three rollback sites reduce to block.length = 0. Drop the variable, or make begin genuinely resumable if that was the intent.
  • bridgeClient.ts:71 duplicates the 100 000-code-unit bound as MAX_CHANNEL_DELIVERY_TEXT_CHARS, independent of MAX_CHANNEL_DELIVERY_TEXT_LENGTH in channel-delivery-ipc.ts. The error-code sets got a cross-check test; this didn't. If they drift, handleChannelDelivery throws invalidParams, no channel_delivery_result is published at all, and the only trace is a child-side debugLogger.warn. Add the cross-check.
  • channel-notify.ts:82 — the deliverChannelMessage presence check runs before parseNotifyBody, so a malformed body on a daemon with no worker returns 503 instead of 400. Cheap reorder.
  • channel-worker-group.ts:719deliverChannelMessage(request, workspaceCwd?) still supports workspaceCwd === undefined → "first worker that owns the channel". Nothing calls it that way today, but the design explicitly forbids cross-workspace fallback; making the parameter required would prevent it from being reintroduced by accident.
  • Session.ts:3769#scheduleChannelDelivery(params: Record<string, unknown>) erases the type, so neither call site is checked against ChannelDeliveryInfo. Worth typing given the correlation fields are validated on the far side.
  • ChannelDeliveryAuthorizationStore scheduled entries are only removed on explicit DELETE/clear-delivery; the keepalive onTasksRead re-registration never prunes tasks that vanished from the file (out-of-band edits, external cleanup). Consider reconciling against the read set.

Verification note

The Tested-on matrix marks Linux and Windows ⚠️. Given this PR touches child-process IPC, worker shutdown draining, and the cron scheduler, it'd be worth at least one Linux run of the notify + scheduled paths before merge.

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Review — feat(daemon): add explicit channel delivery @ 464baba

Reviewed the full diff (64 files, ~1.6k production lines) with a worktree at head. The architecture holds up: one public {kind:'channel', target} shape, one daemon-side authorization store, one worker IPC path ending at ChannelBase.deliverProactive(), and three producers that keep their own execution semantics. Trust boundaries are right — bridge.ts strips caller-supplied delivery/_meta, createBoundChannelDeliveryHandler closes over the canonical workspace and consumes authorization before worker lookup, and routeEntry(name, workspaceCwd) has no cross-workspace fallback.

Confirmed closed at head (re-verified in source, not just by reply): typed ChannelProactiveDeliveryError classification replaced the message-regex in daemon-worker.ts; permanent adapter rejection now maps to channel_delivery_rejected/502 rather than 400; shutdown drains deliveries and webhook tasks concurrently under one WORKER_SHUTDOWN_DRAIN_MS; PATCH accepts/clears delivery and re-pins authorization; Session.ts reuses the CLI normalizeChannelDeliveryText helper; the negative Session paths (cancel / max_tokens / stream failure, prompt and scheduled) are now asserted.

Findings below are new.


1. [High] armedDurableOneShots skip is unbounded — a durable one-shot whose tick missed its slot is stranded forever

packages/core/src/services/cronScheduler.ts:832-837

if (
  !t.recurring &&
  this.timer !== null &&
  this.armedDurableOneShots.has(t.id)
)
  continue;

The comment scopes the intent to a sub-second gap ("reload … before this armed job's next 1s tick"), but the predicate carries no staleness bound. Combined with processJob(), which only matches slots inside ±ceil(|jitter|/60_000) minutes of the current minute (cronScheduler.ts:1489-1500), an armed one-shot whose tick never observed its slot can never fire and is now permanently excluded from missed-detection: the install loop re-arms it on every reload, so the skip is re-applied indefinitely. The set is only cleared on fire, removal, disappearance from disk, #shouldFireDurable flipping false, or stop().

Trigger is any stall that spans the slot minute while the scheduler stays up: laptop suspend/resume (the common case for a local daemon), a clock jump, or event-loop starvation from a long synchronous tool run in the same child.

Repro (added to cronScheduler.test.ts, durable ownership) — passes on this head, i.e. nothing fires and the task stays on disk:

it('REPRO strands an armed one-shot whose tick never observed the slot', async () => {
  vi.useFakeTimers();
  try {
    const createdAt = new Date(2025, 0, 15, 10, 14, 0).getTime();
    vi.setSystemTime(new Date(2025, 0, 15, 10, 14, 30));
    await writeCronTasks(tmpDir, [{
      id: 'stalledOneShot', cron: '15 10 * * *', prompt: 'armed prompt',
      recurring: false, createdAt, lastFiredAt: null, sessionId: 'sess-A',
    }]);
    const fired: CronJob[] = [];
    scheduler.start((job) => fired.push(job));
    await scheduler.enableDurable('sess-A');

    // Process suspended across 10:15 — no tick ran inside the slot.
    vi.setSystemTime(new Date(2025, 0, 15, 11, 0, 0));
    await (scheduler as unknown as {
      loadFileTasks(handleMissed: boolean): Promise<void>;
    }).loadFileTasks(true);
    scheduler.tick();

    expect(fired).toHaveLength(0);                                  // never fires
    expect((await readCronTasks(tmpDir)).map((t) => t.id))
      .toEqual(['stalledOneShot']);                                 // never cleaned up
  } finally { vi.useRealTimers(); }
});

Negative control: deleting only the 6-line guard makes the same test fail with expected [ { id: 'stalledOneShot', … } ] to have a length of +0 but got 1 — i.e. before this PR the reload delivered the batched missed-task notification and removed the task. This is a regression in the missed-one-shot safety net, and it applies to all durable one-shots, not only delivery-enabled ones.

Suggested fix — bound the skip to the window the comment actually describes, after nextFire is known:

const nextFire = computeNextFireMs(t.cron, anchor, jitter);
if (nextFire === null || nextFire >= now) continue;
if (
  !t.recurring &&
  this.timer !== null &&
  this.armedDurableOneShots.has(t.id) &&
  now - nextFire < 60_000            // one tick-slot of slack, not unbounded
)
  continue;

I applied exactly this in the worktree: cronScheduler.test.ts + cronTasksFile.test.ts = 168/169 pass, the only failure being the REPRO above (which asserts the buggy behaviour). The PR's own does not classify an armed bound one-shot as missed on watcher reload (0.5 s stale) still passes. Worth adding a companion test for the long-stall case.


2. [Medium] Delivery error strings are published verbatim into channel_delivery_result — including the daemon's absolute workspace path

packages/cli/src/serve/run-qwen-serve.ts:253 vs :210

The handler is careful to strip the recipient id from the log:

diagnosticText = sanitizeWorkerDiagnostic(
  info.target.id.length > 0 ? message.replaceAll(info.target.id, '<redacted>') : message, );

…but the value that actually reaches subscribers gets no such treatment:

if (isChannelDeliveryError(err)) {
  return failed(err.code, err.message);   // ← err.message → host result → SSE event
}

bridgeClient.handleChannelDelivery only length-caps it (500 chars) and republishes it as data.error. Concretely, channel-worker-group.ts:724 builds:

`No channel worker for workspace "${workspaceCwd}" owns channel "${request.channelName}".`

so any prompt/scheduled delivery to a workspace whose worker doesn't own the channel publishes the daemon's absolute filesystem path into the session event stream and back to the child. The repo is otherwise careful about this (cf. the loop.md resolution failed sanitization in Session.ts, added precisely because "re-throwing the raw fs error would leak that absolute path"), and docs/design/channel-delivery-v1.md promises the event carries only "sanitized error data". Same channel can carry provider text that echoes a recipient id (DingTalk's HTTP ${status} ${detail} includes 300 chars of response body).

Suggestion: run the same replaceAll(info.target.id, …) + sanitizeWorkerDiagnostic over the error string returned from failed(), and have channel-worker-group use a workspace-agnostic public message (keep the cwd in the daemon log only).


3. [Low-Medium] channel_delivery_rejected is effectively Feishu-only

refactor(channels): defer DingTalk error classification reverted DingTalk's typed dispositions, so every DingTalk failure — invalid recipient, flow-controlled recipient, HTTP 4xx, bad app credentials — is wrapped transient by ChannelBase.pushProactiveDelivery and reports channel_delivery_failed. Feishu maps the identical conditions to permanentchannel_delivery_rejected. The design doc does cover this ("except adapters that already provide a typed permanent disposition"), but the protocol doc, the SDK error-code union and the PR body all present channel_delivery_rejected as a general contract, and the E2E table in the description claims a DingTalk "sanitized permanent delivery error" — which the current head cannot produce. Worth either a one-line caveat in qwen-serve-protocol.md naming which adapters can emit it, or restoring the DingTalk mapping.

4. [Low] Scheduled tasks accept delivery on a task that can never deliver

packages/cli/src/serve/routes/scheduled-tasks.ts:530 (create) and :783 (PATCH) both gate on task.delivery && task.sessionId, and durableTaskToJob only threads delivery when task.sessionId is set. When the CRUD routes run without a bridge (bridge absent / manageScheduledTaskSessions: false), the task is created unbound: delivery is validated, persisted, echoed back in the 201/GET view, no authorization is registered, and nothing ever fires or emits a channel_delivery_result. The scheduler test does not propagate delivery to the fired job for unbound tasks pins this as intended, but from the API side it's a silent no-op. A 400 at creation when delivery is present and no session will be bound would be cheap and self-documenting. (run-qwen-serve passes manageScheduledTaskSessions: true, so this is embedder-facing today.)

5. [Low] channel_delivery is advertised unconditionally

capabilities.ts:285 registers channel_delivery: { since: 'v1' } with no entry in the toggle table, unlike the sibling channel_control (:514, gated on channelControlAvailable). Deliberate per the doc ("protocol support, not a live-health assertion"), and fine for qwen serve. But in a createServeApp embedding without deliverChannelMessage/channelDeliveryAuthorizations, a client that pre-flights the capability gets notify → permanent 503 and prompt deliveryno channel_delivery_result at all (the extMethod resolves methodNotFound, which Session's .catch swallows). Consider gating on deps.deliverChannelMessage !== undefined, or documenting that the capability is unconditional.

6. [Low] Feishu: the pre-existing webhook path lost its original error text

FeishuAdapter.ts:785-792 — the catch now discards the underlying error and throws ChannelProactiveDeliveryError('transient', 'Feishu sendMessage failed: network error', { cause: err }). pushProactive (:687, throwOnFailure: true) is the pre-existing webhook/lifecycle path, whose lastError previously carried the real message (fetch failed, TimeoutError, …). The stderr line still logs ${err} and cause is preserved, but anything reading .message now sees a constant. Cheap fix: append the original message.

7. [Nit] No cross-package cross-check for the 100 k text bound

The error-code set has a real cross-check test (sdk events.ts copy vs. the canonical set). The text bound doesn't: MAX_CHANNEL_DELIVERY_TEXT_CHARS (bridgeClient.ts:71, a rejection bound) and MAX_CHANNEL_DELIVERY_TEXT_LENGTH (channel-delivery-ipc.ts, a truncation bound) are independent. Raising the CLI one alone makes handleChannelDelivery throw invalidParams, which Session.#scheduleChannelDelivery's .catch swallows — delivery silently vanishes with no event. One assertion mirroring the error-code cross-check would close it.


Other notes (no action required)

  • Target-id semantics are platform-specific and undocumented. DingTalk type:'user' needs a staff userId and type:'chat' an openConversationId; Feishu user is routed as open_id and chat as chat_id (FeishuAdapter.ts:690-700). With contact discovery explicitly out of scope, one sentence per adapter in qwen-serve-protocol.md would save callers a round of 502s.
  • deliverProactive deliberately widens the proactive target set — DingTalk's supportsProactiveTarget is group-only, while the new supportsProactiveDeliveryTarget admits DMs (mirroring the webhook override). That's the point of the feature, but it does mean any bearer-authenticated daemon caller can DM an arbitrary staff id without any per-channel allowlist. Worth stating explicitly in the docs as the security posture.
  • Test coverage is genuinely strong — the final-block-only capture (tool preamble discarded, continuation replaces the candidate, non-continuation retry rolls back), authorization consume-once / monotonic-fire, exact-workspace routing with no fallback, IPC send-throw and send-callback error paths, and the 16-way queue-full cap all have targeted tests. The remaining untested seams I'd add: the 60 s CHANNEL_DELIVERY_AUTHORIZATION_GRACE_MS prompt revoke in routes/session.ts:2637-2646 (fake timers), and revokeScheduledTask on DELETE.

Verdict

No merge blocker in the delivery feature itself — the boundary design is sound and the previously-raised threads are genuinely closed. Finding 1 should be fixed before merge: it's in packages/core, it silently affects every durable one-shot scheduled task rather than just delivery-enabled ones, and the fix is a one-line staleness bound that keeps all 168 existing cron tests green. Findings 2–3 are worth folding in while the branch is open; the rest can be follow-ups.

中文说明

在 head 464baba 上用独立 worktree 复核了完整 diff。整体架构是站得住的:统一的 {kind:'channel', target} 公共契约、daemon 侧授权存储、单一 Worker IPC 路径直到 ChannelBase.deliverProactive(),三个生产者各自保留执行语义。信任边界处理正确——bridge.ts 剥离调用方伪造的 delivery/_metacreateBoundChannelDeliveryHandler 闭包绑定规范 workspace 且在查找 Worker 之前消费授权,routeEntry 无跨 workspace 回退。

已确认在当前 head 关闭(逐条回源码验证,非仅凭回复):typed ChannelProactiveDeliveryError 分类取代消息正则;永久性拒绝映射到 channel_delivery_rejected/502 而非 400;关停时投递与 Webhook 任务在同一个 WORKER_SHUTDOWN_DRAIN_MS 预算下并发排空;PATCH 支持设置/清除 delivery 并重新绑定授权;Session.ts 复用 CLI 的 normalizeChannelDeliveryText;取消 / max_tokens / 流失败的负向路径(Prompt 与定时)均已断言。以下为新发现。

1.【高】armedDurableOneShots 跳过没有时效上限,错过时隙的一次性任务会被永久搁浅cronScheduler.ts:832-837)。注释描述的是「亚秒级」窗口,但判断条件没有任何 staleness 边界;而 processJob 只匹配当前分钟 ±ceil(|jitter|/60000) 分钟内的时隙,因此一旦 tick 没有落在时隙内(笔记本休眠/唤醒、时钟跳变、同进程长同步工具调用导致事件循环饥饿),该任务既永远不会被 tick 触发,又因为每次 reload 都会被重新 arm 而永远被 missed 检测跳过——既不执行也不清理,且不再产生 missed 通知。我写了复现用例(见上方英文代码块):在当前 head 通过(说明确实被搁浅);负向对照——只删掉这 6 行 guard,同一用例即失败(got 1),即改动前 reload 会投递批量 missed 通知并从磁盘移除。这是对 missed 安全网的回归,且影响所有持久化一次性任务,不限于带 delivery 的。建议把跳过限制在注释真正描述的窗口内(把判断移到 nextFire 计算之后并加 now - nextFire < 60_000)——我在 worktree 里验证过:cronScheduler + cronTasksFile 共 168/169 通过,唯一失败的就是上面这个断言 buggy 行为的复现用例,PR 自带的 0.5 秒用例仍然通过。

2.【中】投递失败的 error 字符串未脱敏就发布进 channel_delivery_result,其中包含 daemon 的绝对路径。 run-qwen-serve.ts:210 会把 info.target.id日志里替换掉,但 :253failed(err.code, err.message) 直接把原始 message 作为事件字段返回;bridgeClient 只做 500 字符截断。而 channel-worker-group.ts:724 构造的消息里带有 workspaceCwd 绝对路径,因此任意一次「该 workspace 的 Worker 不拥有该 channel」的 Prompt/定时投递,都会把 daemon 的绝对文件路径发布到 session 事件流并回传给子进程。仓库其他地方对此很谨慎(参见 Session.tsloop.md resolution failed 的脱敏注释),设计文档也承诺事件只含「sanitized error data」。建议对返回的 error 走同样的 replaceAll + sanitizeWorkerDiagnostic,并让 group 的对外消息不含 cwd。

3.【中低】channel_delivery_rejected 实际上只有飞书能产生。 refactor(channels): defer DingTalk error classification 回退了钉钉的 typed disposition,所以钉钉的无效接收人、限流、HTTP 4xx、凭证错误全部被 ChannelBase.pushProactiveDelivery 包成 transientchannel_delivery_failed;飞书对同类条件返回 permanentchannel_delivery_rejected。设计文档有铺垫,但协议文档、SDK 错误码联合类型和 PR 描述都把它当作通用契约,且描述里的 E2E 表格声称钉钉产生了「永久性投递错误」——当前 head 做不到。建议协议文档加一句说明哪些适配器能产生,或恢复钉钉映射。

4.【低】未绑定 session 的定时任务会接受并持久化一个永远不会投递的 deliveryscheduled-tasks.ts:530/:783 都以 task.delivery && task.sessionId 为条件),201/GET 仍会回显该字段,但既不注册授权也不会触发或发事件。建议在无法绑定 session 时直接 400。

5.【低】channel_delivery 能力无条件广告capabilities.ts:285),而同族的 channel_control 是按 channelControlAvailable 开关的(:514)。对 qwen serve 无影响,但在缺少相关 deps 的 createServeApp 嵌入场景下,预检能力的客户端会得到「notify 恒 503 + Prompt delivery 完全没有事件」。

6.【低】飞书既有 Webhook 路径丢失了原始错误文本FeishuAdapter.ts:785-792):catch 现在统一抛出常量 'Feishu sendMessage failed: network error'pushProactive(既有路径)的 lastError 因此变成固定串。cause 有保留,建议把原始 message 拼接回去。

7.【建议】100k 文本上限缺少跨包一致性测试。 错误码集合有 cross-check 测试,文本上限没有:bridgeClientMAX_CHANNEL_DELIVERY_TEXT_CHARS(拒绝阈值)与 CLI 的 MAX_CHANNEL_DELIVERY_TEXT_LENGTH(截断阈值)相互独立,单独调高 CLI 侧会让 handleChannelDeliveryinvalidParams,而 Session.catch 会吞掉——投递静默消失且无事件。

其他(无需处理): 目标 id 的语义随平台而异(钉钉 user=staff userId、chat=openConversationId;飞书 user→open_id、chat→chat_id),协议文档里各写一句会省掉调用方一轮 502;deliverProactive 有意放宽了主动发送目标集合(钉钉常规主动路径仅限群,投递路径允许单聊),这是功能本意,但也意味着任何持 bearer 的调用方可以向任意 staff id 发起单聊,建议在文档里把这一安全姿态写明;测试覆盖整体很扎实,建议补的两处是 routes/session.ts:2637-2646 的 60 秒授权回收计时器和 DELETE 时的 revokeScheduledTask

结论: 投递特性本身没有合并阻塞项,边界设计合理,此前的 review 线程确实已闭环。建议合并前修掉发现 1——它落在 packages/core,静默影响所有持久化一次性任务而不只是带投递的,且修法是一行 staleness 边界、168 个既有 cron 测试全绿。发现 2–3 建议趁分支还开着一并处理,其余可作为后续。

Reviewed by Claude Opus 4.8 via Claude Code

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Local Build + Real Daemon Review Evidence for PR #7388

Reviewer: wenshao (maintainer)
Commit under test: 47629ed0f2efa9e485b143ca123d93c3e5be9f3e (PR head)
Platform: macOS (darwin), Node.js v22.22.2, npm 10.9.7
Worktree: isolated .qwen/worktrees/quick-oak-9c54f4

Build & Typecheck

Step Command Result
Build npm run build ✅ PASS (serial, no concurrent dist races)
Bundle npm run bundle ✅ PASS — dist/cli.js produced
Typecheck npm run typecheck ✅ PASS (all 9 workspaces)

Note: The first parallel build attempt failed in acp-bridge due to a concurrent dist/ cleanup race between two processes in the same worktree (the build_package.js script deletes dist/ before tsc --build). A clean serial rebuild succeeded without any code changes. This is an environment concurrency issue, not a PR defect.

Targeted Unit Tests

Package Test Files Tests Result
channels/base ChannelBase.test.ts 506
channels/dingtalk DingtalkAdapter.test.ts 84
channels/feishu adapter.test.ts 75
core cronScheduler.test.ts + cronTasksFile.test.ts 168
sdk-typescript DaemonClient.test.ts + daemonEvents.test.ts + daemon-public-surface.test.ts 414
cli (serve) channel-delivery-authorization + channel-delivery-ipc + channel-worker-* + daemon-worker 236
Total 1,483 ✅ All passed

Known macOS limitation (pre-existing, not introduced by this PR): 4 CLI test files (channel-delivery.test.ts, channel-notify.test.ts, scheduled-tasks.test.ts, scheduled-task-keepalive.test.ts) fail at import time with TypeError: Cannot destructure property 'Terminal' of 'default' as it is undefined — a CJS/ESM interop issue with @xterm/headless on macOS that also reproduces on main. These same files pass on Ubuntu CI.

Real Daemon E2E Verification (Built Bundle)

Started the built dist/cli.js serve with isolated HOME, QWEN_RUNTIME_DIR, fake model endpoint, and no Channel Worker configured. Verified the public HTTP contract directly:

Scenario Endpoint Expected Actual Status
Health probe GET /health 200 200 {"status":"ok"}
Capability negotiation GET /capabilities includes channel_delivery channel_delivery: true, 99 features
Notify without auth POST /workspace/notify 401 401 {"error":"Unauthorized"}
Notify with invalid body POST /workspace/notify 400 + channel_delivery_invalid 400 + channel_delivery_invalid
Notify without worker POST /workspace/notify 503 + channel_worker_unavailable 503 + sanitized message
Notify unknown workspace POST /workspaces/missing/notify 400 + workspace_mismatch 400 + workspace_mismatch

Key observations:

  • The daemon advertises channel_delivery in capabilities, confirming protocol support.
  • Missing auth returns 401 before any delivery logic runs.
  • Invalid payloads are rejected with channel_delivery_invalid before reaching worker IPC.
  • When no Channel Worker is running, the daemon returns 503 channel_worker_unavailable with a sanitized error message — no internal details leak.
  • Unknown workspace paths return 400 workspace_mismatch, not a 500 or stack trace.
  • No lazy worker startup or fallback to primary runtime occurs on missing workers (matches design doc).

Screenshot Evidence

Terminal capture of the real daemon API responses (built bundle, isolated environment):

PR 7388 Channel Delivery — Real Daemon API Verification

Screenshot branch: wenshao/qwen-code@pr-7388-local-review-assets

Scope & Limitations of This Verification

  • Verified: Build, typecheck, targeted unit tests (1,483), real daemon HTTP contract (capability, auth, validation, workspace routing, worker-unavailable error semantics).
  • Not verified locally (requires real IM credentials): End-to-end message delivery to DingTalk/Feishu, prompt final delivery after Agent turn completion, scheduled-task one-shot delivery. These paths are covered by unit tests and the author's real DingTalk Stream evidence in the PR description.
  • Not verified locally (macOS CJS/ESM issue): 4 CLI test files that transitively import @xterm/headless — same issue on main, passes on Ubuntu CI.

Merge Recommendation

From a local build + real daemon perspective, this PR is ready to merge. The public API contract is correctly implemented: capability advertisement, authentication gating, input validation, workspace-scoped routing, and sanitized error codes all match the design document. The 1,483 targeted unit tests pass, and the built bundle behaves correctly under real HTTP probing. The remaining unverified paths (real IM delivery) are covered by the author's evidence and CI.


中文报告

PR #7388 本地构建 + 真实 Daemon 验证报告

审查者: wenshao(维护者)
测试提交: 47629ed0f2efa9e485b143ca123d93c3e5be9f3e(PR 头提交)
平台: macOS (darwin),Node.js v22.22.2,npm 10.9.7
工作树: 隔离的 .qwen/worktrees/quick-oak-9c54f4

构建与类型检查

步骤 命令 结果
构建 npm run build ✅ 通过(串行,无并发 dist 竞态)
打包 npm run bundle ✅ 通过 — 生成 dist/cli.js
类型检查 npm run typecheck ✅ 通过(全部 9 个 workspace)

说明: 首次并行构建在 acp-bridge 失败,原因是同一 worktree 中两个进程并发执行 build_package.js(该脚本在 tsc --build 前删除 dist/),导致竞态。串行重建后无任何代码修改即成功。这是环境并发问题,不是 PR 缺陷。

定向单元测试

测试文件 测试数 结果
channels/base ChannelBase.test.ts 506
channels/dingtalk DingtalkAdapter.test.ts 84
channels/feishu adapter.test.ts 75
core cronScheduler.test.ts + cronTasksFile.test.ts 168
sdk-typescript DaemonClient.test.ts + daemonEvents.test.ts + daemon-public-surface.test.ts 414
cli (serve) channel-delivery-authorization + channel-delivery-ipc + channel-worker-* + daemon-worker 236
合计 1,483 ✅ 全部通过

已知 macOS 限制(已有问题,非本 PR 引入): 4 个 CLI 测试文件(channel-delivery.test.tschannel-notify.test.tsscheduled-tasks.test.tsscheduled-task-keepalive.test.ts)在导入时因 @xterm/headless 的 CJS/ESM 互操作问题失败,该问题在 main 分支同样复现。这些文件在 Ubuntu CI 上通过。

真实 Daemon 端到端验证(构建产物)

使用构建后的 dist/cli.js serve 启动隔离 daemon(独立 HOMEQWEN_RUNTIME_DIR、假模型端点、未配置任何 Channel Worker),直接验证公开 HTTP 契约:

场景 端点 预期 实际 状态
健康探针 GET /health 200 200 {"status":"ok"}
能力协商 GET /capabilities 包含 channel_delivery channel_delivery: true,99 个特性
无鉴权通知 POST /workspace/notify 401 401 {"error":"Unauthorized"}
非法 body 通知 POST /workspace/notify 400 + channel_delivery_invalid 400 + channel_delivery_invalid
无 Worker 通知 POST /workspace/notify 503 + channel_worker_unavailable 503 + 脱敏消息
未知 workspace POST /workspaces/missing/notify 400 + workspace_mismatch 400 + workspace_mismatch

关键观察:

  • Daemon 在 capabilities 中通告 channel_delivery,确认协议支持。
  • 缺少鉴权时在任何投递逻辑之前返回 401。
  • 非法 payload 在到达 worker IPC 之前被 channel_delivery_invalid 拒绝。
  • 未运行 Channel Worker 时,daemon 返回 503 channel_worker_unavailable 且错误消息已脱敏 — 无内部细节泄露。
  • 未知 workspace 路径返回 400 workspace_mismatch,不是 500 或堆栈跟踪。
  • 缺失 worker 时不会懒启动或回退到 primary runtime(与设计文档一致)。

截图证据

真实 daemon API 响应的终端截图(构建产物,隔离环境):

PR 7388 Channel Delivery — 真实 Daemon API 验证

截图分支:wenshao/qwen-code@pr-7388-local-review-assets

本次验证的范围与限制

  • 已验证: 构建、类型检查、定向单元测试(1,483 条)、真实 daemon HTTP 契约(能力通告、鉴权、校验、workspace 路由、worker 不可用错误语义)。
  • 本地未验证(需要真实 IM 凭据): 到钉钉/飞书的端到端消息投递、Agent 回合完成后的 prompt final 投递、定时任务一次性投递。这些路径由单元测试和 PR 描述中作者的真实钉钉 Stream 证据覆盖。
  • 本地未验证(macOS CJS/ESM 问题): 4 个间接导入 @xterm/headless 的 CLI 测试文件 — main 上同样存在此问题,Ubuntu CI 通过。

合入建议

从本地构建 + 真实 daemon 角度看,本 PR 可以合入。公开 API 契约实现正确:能力通告、鉴权门控、输入校验、workspace 作用域路由和脱敏错误码均与设计文档一致。1,483 条定向单元测试通过,构建产物在真实 HTTP 探测下行为正确。剩余未验证路径(真实 IM 投递)由作者证据和 CI 覆盖。

wenshao
wenshao previously approved these changes Jul 23, 2026
qqqys
qqqys previously approved these changes Jul 23, 2026
return JSON.stringify([workspaceCwd, sessionId, id]);
}

function targetsEqual(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] JSON.stringify([workspaceCwd, sessionId, id]) can produce key collisions if field values contain ","\" or "[". Use length-prefixed concatenation instead: ${workspaceCwd.length}:${workspaceCwd}/${sessionId.length}:${sessionId}/${id}`.

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

复核当前 head 464baba 后,此项不成立,因此不修改实现。

源码证据:authorizationKey 的三个入参均为 string,key 是固定三元素字符串数组的 JSON 序列化。JSON 字符串会转义逗号、引号、反斜杠和方括号;JSON.parse(key) 可以无歧义还原原始三元组,因此这些字符不会改变数组元素边界。

额外执行证据:用 "", ,, ", [, ], \\, a, \n 组成全部 512 个三元组,得到 512 个唯一 key,逐个 JSON round-trip 均保持原值,collision 为 null。长度前缀方案不会提升这里的正确性,反而增加自定义编码逻辑。

authorization.sessionId,
authorization.deliveryId,
),
authorization.target,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] #prompts Map entries that are never consumed (e.g. session crashes before consume) accumulate indefinitely — memory leak risk. Add TTL-based cleanup or batch revoke on session end.

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

复核当前 head 464baba 后,评论给出的“session crash 后永久累积”路径不成立,因此不增加 TTL。

源码证据:Prompt admission 同步失败时会立即 revokePrompt;已接收的 promptPromise 无论 resolve/reject 都会在 finally 中安排 60 秒 grace 后撤销;成功消费则由 consume 立即删除

Crash 行为也有现成回归测试:queued prompt rejects when the channel crashes before it starts 同时验证 active/queued prompt 均 reject,session/pending count 清零。我在该 head 单独重跑:1 passed / 438 skipped。默认上限还是每 workspace 20 sessions、每 session 5 pending prompts。一个仍在运行且未配置 deadline 的长 Prompt 可以保留授权,但那是活跃请求而非遗失条目;任意 TTL 反而会让合法长任务的最终投递失效。

if (typeof value !== 'object' || value === null) return false;
const request = value as Record<string, unknown>;
return (
isNonEmptyString(request['deliveryId']) &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] No queue size limit on pendingChannelDeliveries — a legitimate but high-volume caller can grow this Map unbounded. Add a MAX_PENDING cap and return channel_delivery_queue_full when exceeded.

— qwen3.8-max-preview via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

确认此项成立,将按合入阻塞项修复。

源码证据:supervisor 的 pendingChannelDeliveries 当前没有容量门槛;每次调用会在创建 timer 后直接登记 Map 并发送 IPC。worker 侧虽然有 16 个 active delivery 上限,但这个检查发生在消息抵达 worker 之后,约束不了 supervisor Map 和 Node IPC backlog。

修复边界:在 supervisor 分配 message/timer/Promise 之前应用与 worker 一致的共享 16 in-flight 上限,超限复用现有 channel_delivery_queue_full;补回归测试验证第 17 个请求不进入 IPC,并验证 result/timeout/worker-exit 后容量可释放。不会改变 best-effort、无重试或 HTTP 503 契约。

@BenGuanRan BenGuanRan Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已在 8b3f5f8a9 修复并推送。

源码证据:

验证:相关 CLI 5 个测试文件 400/400 通过;build、typecheck、lint 通过。

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid architecture with clear separation (route → authorization → manager → group → supervisor → IPC → adapter). Security model is sound with one-time tokens and strict key validation. 3 P1 findings posted inline: authorization key collision risk, prompt map memory leak, and missing queue size limit.

— qwen3.8-max-preview via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

— qwen3.7-max via Qwen Code /review

Comment on lines +802 to +804
"status": "failed",
"promptId": "prompt-1",
"code": "channel_worker_unavailable",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] promptId appears at both the top-level SSE envelope and inside data in both channel_delivery_result event examples, with no explanation of whether the two values are always identical or can differ. — Concrete cost: a client implementer cannot determine which to use for correlation. If they assume the two can differ and build separate handling, they add unnecessary complexity.

Consider adding a one-line note explaining the duplication (e.g., "The top-level promptId is the SSE correlation key; data.promptId is always identical for prompt-sourced events") or removing the inner promptId from data if redundant.

— qwen3.7-max via Qwen Code /review

Comment on lines +1585 to +1587
it('delivers existing text on the matching channel without an agent turn', async () => {
const sdk = createSdk();
const deliverProactive = vi.fn().mockResolvedValue(undefined);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test exercises the channel_worker_unavailable error path through deliverChannelMessage. The success path is tested, but a regression that silently succeeds or throws a different error type when the channel is unavailable would not be caught. — Concrete cost: the error classification in classifyChannelDeliveryError (daemon-worker.ts ~line 1090) maps this to channel_delivery_failed, but without a test, a refactor could change the mapping silently.

it('rejects delivery for channels that are not running', async () => {
  const sdk = createSdk();
  const handle = await runChannelDaemonWorker({ /* ... */ });
  await expect(
    handle.deliverChannelMessage({ ...deliveryRequest, channelName: 'slack' }),
  ).rejects.toThrow(/Channel "slack" is not running/);
});

— qwen3.7-max via Qwen Code /review

'workspace_extensions',
'session_branch',
'workspace_reload',
'channel_delivery',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This integration test file is outside every npm workspace, so npm test never collects it. It only runs via explicit CI integration test commands (e.g., test:integration:cli:sandbox:none). — Concrete cost: the Integration Tests (CLI, No Sandbox) CI check is currently skipped for this PR, meaning this test ships without any automated gate at this commit. Confirm the CI job that exercises this file is not among those that get skipped.

— qwen3.7-max via Qwen Code /review

Comment on lines +773 to +775
resp.status === 408 || resp.status === 429 || resp.status >= 500
? 'transient'
: 'permanent',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] HTTP 401 is classified as 'permanent' even though line 766 clears this.tokenCache = undefined on 401, preparing for a fresh token fetch. — Failure scenario: today no consumer branches on disposition for retry, so this is latent. When retry logic is added that respects disposition, a stale-token 401 will be treated as unrecoverable even though the cache was already invalidated and a fresh token is available — the code's own recovery action contradicts its error signal.

Suggested change
resp.status === 408 || resp.status === 429 || resp.status >= 500
? 'transient'
: 'permanent',
resp.status === 401 || resp.status === 408 || resp.status === 429 || resp.status >= 500
? 'transient'
: 'permanent',

— qwen3.7-max via Qwen Code /review

@BenGuanRan
BenGuanRan dismissed stale reviews from qqqys and wenshao via 8b3f5f8 July 23, 2026 14:47
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

合入前修复结论(head 8b3f5f8a9

@wenshao @yiliang114 感谢两位的源码复核。本轮只处理已确认的合入风险,没有扩展 IM adapter 特判或改变 best-effort 设计。

1. armed durable one-shot 不再永久搁浅

  • cronJitterWindowMinutesprocessJob 共用同一个 ceil(abs(jitter)/60s) 分钟窗口。
  • reload 只在原始 cron slot 仍可被下一次 tick 扫描到时保留 armed guard;一旦越过该窗口,就恢复既有 missed one-shot 分支:cronScheduler.ts#L845-L868。这不是固定 60 秒判断,避免负 jitter 的 :00/:30 one-shot 被提前误判为 missed。
  • 新增双向边界测试:越过窗口时触发 missed 并清盘;仍在负 jitter tick 窗口内时保留原始投递:cronScheduler.test.ts#L1452-L1527

2. replayable delivery error 不再携带 workspace/target/credential

3. supervisor 入口增加共享 16 in-flight 上限

已在对应 inline thread 补充逐行证据。supervisor 在 message allocation/send 前拒绝第 17 个请求,worker 复用同一常量作为第二层防护;完成、失败、超时和 worker 退出均释放容量。

验证

  • npm run build:通过
  • npm run typecheck:通过
  • npm run lint:通过
  • Core:cronScheduler.test.ts + cronTasksFile.test.ts,170/170 通过
  • CLI:channel-worker-supervisorchannel-worker-grouprun-qwen-servechannel-notifydaemon-worker,400/400 通过
  • 提交钩子:Prettier + ESLint 通过
  • 独立增量复核:原 credential overlap Critical 与固定文案 Important 均已关闭,未发现新问题

方便时辛苦两位基于当前 head 再确认一下;如果以上边界符合预期,请重新提交正式 review。

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

@qwen-code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review


```json
{ "stopReason": "end_turn" }
{ "promptId": "session-id########1", "lastEventId": 42 }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The 202 response example omits the eventEpoch field that the implementation returns.

The implementation at packages/cli/src/serve/routes/session.ts:2654 returns three fields:

res.status(202).json({ promptId, lastEventId, eventEpoch });

but this example and the prose description (202 {promptId,lastEventId}) show only two. A client implementing the protocol from this doc would not know eventEpoch exists — it is the daemon-side epoch used to detect daemon restarts.

Suggested change
{ "promptId": "session-id########1", "lastEventId": 42 }
{ "promptId": "session-id########1", "lastEventId": 42, "eventEpoch": 1 }

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. 3 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

— qwen3.7-max via Qwen Code /review

try {
opts.onTasksRead?.(tasks);
} catch (err) {
log.debug('keepalive: onTasksRead failed', err);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] onTasksRead failures are caught and logged at debug level only. If the callback throws on every heartbeat (e.g. malformed task data in the cron store), the authorization store is never populated for delivery-enabled tasks. All subsequent delivery attempts fail with opaque errors, and operators monitoring warn/error logs see nothing.

Failure scenario: Persistent onTasksRead failure silently prevents all channel deliveries, with the root cause invisible at default log levels.

Suggested fix: Log at warn level, or add a counter that escalates to warn after N consecutive failures.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Code Review — feat(daemon): add explicit channel delivery (#7388)

Re-review focused on the delta since my prior verification report (issuecomment-5058342300), which found the PR merge-ready at head 47629ed0. The head has since advanced by two commits — fix(daemon): harden channel delivery boundaries (8b3f5f8a9) and fix(docs): remove obsolete prettier ignore (464babaf8). This review covers those changes and confirms they don't regress the earlier verdict.

Overview

The PR introduces one explicit Channel delivery contract shared by three producers — direct POST /workspace/notify (synchronous, waits for adapter accept), Prompt finals (async 202 + later channel_delivery_result event), and scheduled-task finals — routing only to the worker owned by the resolved workspace. Delivery is immediate/best-effort with no outbox or retry. Supersedes #7153/#7152.

The new commits are narrow, defensive hardening plus one genuine cron edge-case bug fix. Net delta: 11 files, ~+260/−25.

Delta assessment

1. Cron armed-one-shot recovery (cronScheduler.ts) — real bug fix ✅
Previously an armed durable one-shot (this.timer !== null && armedDurableOneShots.has(id)) was unconditionally skipped during catch-up detection, deferring it to the 1 s tick. If that tick never fired it (stuck timer, large gap), the one-shot could be skipped indefinitely and never recovered as missed. The fix now defers only while the slot is still within the tick's fire window (isCronSlotVisibleToTick(nextFire - jitter, now, jitter)); once the slot is stale it falls through to missed-recovery. The guard is also correctly moved after the nextFire >= now check, so non-overdue jobs still short-circuit. Logic and window math (ceil(|jitter|/60000) minutes, matching the existing fired-window check) are internally consistent, and the extracted cronJitterWindowMinutes helper de-duplicates the prior inline computation. Two new tests pin both branches: stale-window → missed:true + task removed; in-window early-jitter → deferred to tick → fires non-missed.

  • Minor (non-blocking): neither new test advances fake timers to prove the still-pending armed setTimeout doesn't also fire after missed-recovery. That double-fire is prevented by the pre-existing firePersistPending dedup + task removal, and the toHaveLength(1) assertion would catch a same-turn double, so this is a coverage note rather than a defect.

2. Public error surface (run-qwen-serve.ts, channel-worker-group.ts) — info-leak hardening ✅
channelDeliveryPublicError(code) replaces the raw err.message returned to callers with a fixed per-code string, passing the raw err only to internal logging. Paired with dropping the raw workspaceCwd path from the worker-unavailable hint ("the selected workspace"), this stops internal filesystem paths / channel internals from leaking through delivery-result errors. The default arm harmlessly mirrors channel_delivery_failed.

3. Diagnostic redaction ordering (run-qwen-serve.ts) — correctness fix ✅
Target-id redaction now runs against the already-normalized sanitizeWorkerDiagnostic output using normalizeWorkerDiagnostic(info.target.id), instead of replaceAll on the raw message before sanitizing. Since sanitizeWorkerDiagnostic normalizes (strips invisible/control chars) before returning, redacting in that same normalized space closes the gap where a target id abutting invisible characters could evade the old raw-string replace. Output stays bounded (second sanitizeLogText(..., 512) re-truncates after the <redacted> substitution may grow the string).

4. In-flight cap consolidation (channel-delivery-ipc.ts, daemon-worker.ts, channel-worker-supervisor.ts) — clean ✅
The duplicated literal 16 is lifted to a single exported MAX_CHANNEL_DELIVERIES_IN_FLIGHT, and the supervisor now pre-rejects with channel_delivery_queue_full when pendingChannelDeliveries.size >= MAX before IPC send — defense-in-depth ahead of the worker's own activeChannelDeliveries cap. Both layers share the constant; no double-counting.

Risk / scope note

  • .prettierignore removal is unrelated scope. Dropping docs/users/integration-github-action.md (comment: "Auto-generated action inputs — prettier mangles underscores in inline HTML") belongs to a docs cleanup, not a channel-delivery feature PR. CI's format gate is green, so the file is prettier-clean now — but if that doc is still generated by a script, a future regeneration could reintroduce output that either fails the gate or gets its underscores mangled by prettier. Worth confirming the generator emits prettier-clean output (or splitting this into its own commit/PR); low severity either way.

Correctness / conventions / tests / security

  • Correctness: delta is sound; no behavioral change to Notify/Webhook/cancel/error paths.
  • Conventions: kebab-case files, exported shared constant, extracted helpers, Apache headers present — consistent with the codebase.
  • Tests: every production change ships focused coverage (cron +2 cases, supervisor +1, run-qwen-serve +1). Full CI green at 8b3f5f8a9 (Test ubuntu, Serve A/B, web-shell E2E, review-pr, Real daemon E2E).
  • Security: the delta improves posture — path/message leaks removed from client-facing errors, redaction normalized, queue bound enforced earlier.

Verdict

LGTM — merge-ready. The new commits are correct, well-scoped hardening plus a legitimate cron recovery fix, all test-covered with green CI, and consistent with the prior merge-ready verdict. Only actionable item is the cosmetic .prettierignore scope question above (non-blocking).

中文说明

针对 #7388 在上次「可合并」评审(head 47629ed0,报告见 issuecomment-5058342300)之后新增的两个提交做增量复审:fix(daemon): harden channel delivery boundaries8b3f5f8a9)与 fix(docs): remove obsolete prettier ignore。本次仅覆盖增量部分(11 个文件,约 +260/−25),确认不影响此前结论。

增量评估:

  1. Cron 已武装一次性任务的补偿(cronScheduler.ts)— 真实缺陷修复 ✅:此前已武装的一次性任务在补偿检测中被无条件跳过并交给 1s tick;若该 tick 因定时器卡死或大间隔而未触发,任务会被永久跳过、无法作为 missed 恢复。修复改为仅在该 slot 仍处于 tick 触发窗口内时才延后isCronSlotVisibleToTick),slot 过期后落入 missed 恢复。守卫也正确移到 nextFire >= now 检查之后。新增两条测试分别锁定两条分支。次要(非阻塞):两条测试未推进 fake timer 去验证已武装的 setTimeout 在补偿后不会二次触发——该二次触发由既有的 firePersistPending 去重 + 任务删除防护,属覆盖率说明而非缺陷。

  2. 对外错误面(run-qwen-serve.tschannel-worker-group.ts)— 信息泄露加固 ✅channelDeliveryPublicError(code) 用固定的按码字符串替换返回给调用方的 err.message,原始 err 仅用于内部日志;同时从 worker 不可用提示中移除原始 workspaceCwd 路径,避免内部文件系统路径泄露。

  3. 诊断脱敏顺序(run-qwen-serve.ts)— 正确性修复 ✅:目标 id 脱敏改为在已归一化sanitizeWorkerDiagnostic 输出上、用 normalizeWorkerDiagnostic(target.id) 进行,堵住了原先「原始字符串 replace 早于 sanitize」时被不可见字符规避的漏洞;输出仍受 512 上限约束。

  4. 在途上限常量收敛(channel-delivery-ipc.ts 等)— 清爽 ✅:重复字面量 16 收敛为导出的 MAX_CHANNEL_DELIVERIES_IN_FLIGHT;supervisor 在 IPC 发送前即以 channel_delivery_queue_full 预拒绝,与 worker 侧上限形成纵深防御。

风险/范围提示: .prettierignore 删除属无关范围。移除 docs/users/integration-github-action.md(注释:自动生成、prettier 会破坏内联 HTML 下划线)应属文档清理而非本特性 PR。当前格式门禁为绿,说明该文件目前已符合 prettier;但若该文档仍由脚本生成,后续再生成可能产出无法通过门禁、或被 prettier 破坏下划线的内容。建议确认生成器输出符合 prettier(或拆分为独立提交/PR);无论如何均为低严重度。

结论:LGTM,可合并。 增量为正确且范围收敛的加固 + 一处真实的 cron 恢复缺陷修复,均有测试覆盖、CI 全绿,与此前「可合并」结论一致。唯一可执行项为上述 .prettierignore 范围问题(非阻塞)。

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

'Channel worker IPC send failed.',
);
}
if (pendingChannelDeliveries.size >= MAX_CHANNEL_DELIVERIES_IN_FLIGHT) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Good — MAX_CHANNEL_DELIVERIES_IN_FLIGHT now properly caps the supervisor's pending queue. The shared constant in channel-delivery-ipc.ts and its use in daemon-worker.ts eliminates the duplicated local limit. Previous P1 on queue size is resolved.

— qwen3.8-max-preview via Qwen Code /review

const SHUTDOWN_FORCE_CLOSE_MS = 5_000;
const DAEMON_LOG_FORCED_FLUSH_BUDGET_MS = 250;

function channelDeliveryPublicError(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Good — channelDeliveryPublicError() ensures typed delivery failures return sanitized public messages. Combined with normalizeWorkerDiagnostic and the workspace path removal in channel-worker-group.ts, the error surface no longer leaks internal paths or secrets.

— qwen3.8-max-preview via Qwen Code /review

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental changes since last review address 2 of 3 P1 findings: (1) delivery queue now has a size cap (MAX_CHANNEL_DELIVERIES_IN_FLIGHT=16) enforced in supervisor, (2) error messages sanitized — no more workspace paths or secrets in public errors. Also improved cronScheduler one-shot jitter handling. Remaining items (authorization key collision, prompt map cleanup) can be follow-ups.

— qwen3.8-max-preview via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 23, 2026
Merged via the queue into QwenLM:main with commit bfe8ab4 Jul 23, 2026
167 of 168 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

The stage comments above were updated with the latest result. View workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(daemon): add explicit Channel delivery for prompts, schedules, and notifications

7 participants