feat(daemon): deliver scheduled results to explicit channel targets - #7153
feat(daemon): deliver scheduled results to explicit channel targets#7153BenGuanRan wants to merge 40 commits into
Conversation
665fdef to
355179b
Compare
BenGuanRan
changed the title
|
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）为单个提交。 |
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Code Review — round 1 (at
|
ccb0bd5 to
5daa06c
Compare
Code Review — round 2 (at
|
| # | Finding | Status |
|---|---|---|
| 1 | [blocking] static createScheduledDeliveryDispatcher import in run-qwen-serve.ts pulls @qwen-code/qwen-code-core (5.7 MB chunk) into the serve fast path |
Open. CI Test (ubuntu-latest) → “Check serve fast-path bundle closure” still fails on this head (run 29641334551). Fix: move the dispatcher into loadServeRuntimeModules()’s dynamic imports, like scheduled-task-channel-admission.js. |
| 2 | [correctness] cron loop turnAnswer not reset in the RETRY/MODEL_FALLBACK branch → a mid-stream retry delivers a duplicated partial+full answer |
Open (Session.ts — the branch still resets only functionCalls.length = 0). |
| 3 | >100 k-char answers make enqueueScheduledDelivery throw and Session only debug-logs → silent non-delivery |
Open. |
| 4 | Worker silently drops guard-failing channel_delivery IPC → 5× 30 s blind timeouts instead of fast channel_delivery_invalid |
Open. |
| 5 | threadId validation inconsistent (routes/task-file accept '', untrimmed; IPC guard requires non-empty) |
Open. |
| 6 | Delivery retry resends already-delivered chunks (DingTalk/Feishu) | Open — acceptable as follow-up. |
| 7 | deliveryId = taskId:firedAt minute-granularity — document the invariant |
Open — nit. |
| 8 | PR body doesn’t use the CI-enforced template (What this PR does / Why / Reviewer Test Plan / Evidence / Tested on) and lacks the collapsed 中文 section | Open (the round-2 body no longer misattributes the CI failure, but the template and 中文 section are still missing). |
Process note: the branch was rebased and force-pushed mid-review; per repo convention, additive commits keep review deltas auditable (the force-push reminder bot fired on this push).
New work since round 1 — reviewed
Envelope.deliveryChatId split (a8f5971) — correct and coherent end-to-end. DingTalk DMs now observe chatId = senderStaffId (the stable one-to-one send id) while session routing keeps the reply conversation id (handleLoopAdd resolves the router with envelope.*, not the normalized target). Admission, the stored task snapshot, the delivery request, and supportsProactiveTarget all agree on the stable id, and Feishu correctly falls through to the conversation id. Relaxing DingTalk supportsProactiveTarget from group-only to any explicit-isGroup stable id deliberately enables DM loops; since DM /loop add was previously rejected at that same gate, no legacy stored loop can carry a non-routable conversation id, so there’s no migration hazard. Tests cover both the observation and the /loop add target/session divergence.
One-shot reload-race fix (7111c71) — sound. Keeping an already-loaded, self-bound, due-within-60 s one-shot live across a shared-file watcher reload (instead of demoting it to the confirm-first “missed” path) is narrowly scoped by owner + freshness window, and the deliberate expiry of that grace after the due minute is documented and tested.
Web Shell picker (1bd0621, f0df21f) — well built. Sequence-guarded async loads, workspace-switch resets, datalist-based selection with fail-closed exact resolution (ambiguous pasted ids reject), dual capability gating in App.tsx, and EN+ZH i18n. Especially good: an unchanged stored target is omitted from the PATCH, so editing a task whose target has aged out of the 7-day window doesn’t trip re-admission — matching the “display and preserve, but not newly select” contract — and delivery: null is sent only when the user explicitly clears a previously-stored target. The mapper/dialog tests hit the right edges (capability off, exact-id accept/reject, stale preserve+clear, per-workspace reload, ambiguous-id rejection, collision-safe keys).
webui actions (4e9220c, 896fdd0) — pattern-consistent. listObservedChannelContacts matches the existing action style (bearer header, timeout, error surface) and the paths line up with the server registration (/workspace/channel/observed-contacts, /workspaces/:workspace/...).
New minor notes:
- [nit] When editing a task whose target is no longer observed, the input shows the raw composite
saved · <channel> · <chatId>— functional, but it reads as an internal token and isn’t localized. Rendering the samekind · label · descriptionshape (or a localized “saved” prefix) would be cleaner. - [carry-over nit]
DaemonScheduledTask.sessionBinding/deliveryare typed non-optional (| null) but an older daemon simply omits them (undefined); consistent with existing fields in this file, so fine — just noting the capability gate is what actually protects the UI.
Verdict
Still not merge-ready — the two blockers from round 1 (CI bundle-closure break; RETRY answer duplication) remain, and both fixes are small and local. Findings 3–5 should also land in this PR; 6–7 can be follow-ups. The round-2 additions themselves are high quality and E2E evidence is now much stronger; once the round-1 items are addressed this looks close.
中文版本
代码评审 — 第 2 轮（5daa06c）
第 1 轮之后新增的工作质量很好——deliveryChatId 的读/发 ID 分离、一次性任务重载竞态修复、Web Shell 目标选择器设计和测试都很扎实。但第 1 轮的所有发现均未处理（推送时间 10:40Z 与第 1 轮评论 10:10Z 很可能交错），包括两个阻塞项：
- 【阻塞】
run-qwen-serve.ts仍静态导入scheduled-delivery-dispatcher.js，把 5.7 MB core chunk 拉进 serve 快速路径；本头提交上 CI “Check serve fast-path bundle closure” 仍然失败。修复：把 dispatcher 挪进loadServeRuntimeModules()动态导入。 - 【正确性】 cron 循环
RETRY / MODEL_FALLBACK分支仍只清functionCalls，未清turnAnswer，流中途重试会向 IM 投递重复文本。
3–5（10 万字符静默丢投递、worker 静默忽略非法 IPC、threadId三层校验不一致）也仍未处理；6–7 可作后续；PR 描述仍未使用仓库 CI 强制模板，也仍缺折叠中文版本。另外分支在评审期间 rebase 强推，建议评审期间用增量提交。
新增部分的评审结论
deliveryChatId分离：端到端一致（观测、准入、任务快照、投递请求、supportsProactiveTarget均使用稳定 ID；会话路由保留回复会话 ID）。钉钉放开私聊主动发送不构成存量迁移风险（旧网关本就拒绝私聊 loop 创建）。- 一次性任务重载竞态修复：范围收敛（仅限本会话绑定、到期 60 秒内），语义清晰且有测试。
- Web Shell 选择器：序号防竞态、工作区切换重置、datalist + 精确匹配失败关闭、双能力门控、中英文 i18n；未变更的既有目标不重发 PATCH（过期目标可保留不可新选）、仅显式清空才发
delivery: null。测试覆盖到位。 - webui actions：与现有模式一致，路由与服务端注册一致。
- 小问题：编辑过期目标时输入框显示未本地化的
saved · …原始串（外观问题）；sessionBinding/delivery类型未标可选（与现有风格一致，靠能力门控保护）。
结论
仍不可合并：第 1 轮两个阻塞项（CI bundle closure、RETRY 重复文本）待修，改动都很小；3–5 建议本 PR 内处理。第 2 轮新增内容质量高、E2E 证据充分，处理完第 1 轮事项后即接近可合并。
5daa06c to
bd5534b
Compare
BenGuanRan
changed the title
Maintainer Local Verification — PR #7153Verified on the PR head commit ( Build & Type Check
Unit Tests — 2,622 passed, 0 failed
Daemon Smoke TestStarted
SummaryAll 2,622 unit tests across 4 packages pass on the PR branch. Build, bundle, typecheck, and lint are clean. The daemon correctly registers the new
🇨🇳 中文验证报告（点击展开）维护者本地验证 — PR #7153在 PR 头部提交 ( 构建与类型检查
单元测试 — 2,622 通过，0 失败
Daemon 冒烟测试使用构建产物在隔离
总结PR 分支上 4 个包共 2,622 个单元测试全部通过。构建、打包、类型检查、lint 均干净。Daemon 正确注册了新的 结论：本地验证通过，可作为合并参考。 |
Local build & real-test verification — head
|
| # | Round‑2 blocker | Status now | How verified |
|---|---|---|---|
| 1 | Serve fast‑path bundle pulls in ~5.7 MB core | ✅ Fixed | Real bundle build + closure gate passes; esbuild metafile; A/B control |
| 2 | Scheduled delivery duplicates text on a mid‑stream retry/fallback | ❌ Still present | Reproduced with a real test; one‑block fix provided |
One earlier lesser (worker silently dropping guard‑failed deliveries) is now addressed; one minor gap remains. Details below.
✅ Finding #1 — serve fast‑path bundle closure: FIXED and confirmed
The dispatcher is now a type‑only import at module scope (run-qwen-serve.ts:121) and is loaded through await import('./scheduled-delivery-dispatcher.js') in the post‑listen path (run-qwen-serve.ts:~5069). I ran the real gate:
npm run check:serve-fast-path-bundle # clean → build --cli-only → esbuild bundle → gate
→ "Serve fast-path bundle closure check passed." (exit 0)
The esbuild metafile confirms the dispatcher chunk is reachable from the serve pre‑listen chunk only via dynamic-import — no static edge — so core stays out of the fast‑path closure.
A/B control (to prove the gate is load‑bearing, not lax): I reverted the dispatcher import back to static and rebuilt. The gate fails and names a 5.78 MB chunk pulled into the pre‑listen path — Core shell tool runtime (packages/core/src/tools/shell.ts) + glob + @iarna/toml + chokidar + fzf. That is exactly the regression my round‑2 comment described, and the current head avoids it. (Edit reverted afterward; CI Serve A/B is green.)
❌ Finding #2 — delivery duplicates text on a mid‑stream retry / model fallback (still open)
This is the same round‑2 blocker #2; the delivery accumulator block is byte‑identical to 5daa06c.
The cron/delivery loop accumulates the answer by hand (Session.ts ~4443 let turnAnswer = '' … turnAnswer += part.text) and, on RETRY/MODEL_FALLBACK, resets only functionCalls — not turnAnswer (Session.ts:4487‑4497). But a fresh‑restart retry (rate‑limit / transport error) re‑streams the whole answer from scratch. StreamEventType.RETRY's own doc says the consumer "should discard any partial content from the attempt that just failed", and geminiChat.ts even calls popPendingPartialAssistantTurn() before yielding RETRY. Core's Turn.run() resets pendingToolCalls on retry for exactly this reason — the delivery loop does the same for tool calls but forgets the text.
Net effect: the delivered Channel message gets the pre‑retry partial prepended to the final answer. finalAnswer = turnAnswer (Session.ts:4555) → text: finalAnswer (Session.ts:4637).
I added a real test that injects a fresh‑restart RETRY (and a MODEL_FALLBACK) mid‑stream into the existing delivery harness and asserts the correct delivered text. At the PR head both fail:
RETRY→ delivered"daily daily result"(should be"daily result")MODEL_FALLBACK→ delivered"partial final answer"(should be"final answer")
Controlled fix — resetting turnAnswer on a non‑continuation retry/fallback makes both pass, and the full Session.test.ts stays green (370/370, 368 pre‑existing + my 2):
functionCalls.length = 0;
+ // A fresh-restart retry or model fallback re-streams the
+ // whole answer; discard the partial captured so far. A
+ // continuation retry (isContinuation) appends, so keep it.
+ const isContinuation =
+ resp.type === StreamEventType.RETRY &&
+ resp.isContinuation === true;
+ if (!isContinuation) {
+ turnAnswer = '';
+ }
}
Scope note: the Todo‑Stop‑Guard path is immune (it re‑reads getLastModelMessageText()), so tasks that trigger stop‑hook continuations are unaffected; the plain single‑answer delivery path — the default for a hook‑less scheduled task — is the one that corrupts.
Round‑2 lesser findings — status
- ✅ Addressed: the worker now fast‑rejects a guard/permanent proactive‑delivery failure as
channel_delivery_invalid(daemon-worker.ts:1097‑1118) instead of silently dropping it into a 30 s timeout. ⚠️ Still open (minor): anenqueueScheduledDeliverythrow (e.g. answer overMAX_TEXT_LENGTH) is onlydebugLogger.error‑ed (Session.ts:4639‑4644) → silent non‑delivery with no user‑visible signal. Low‑severity; worth a follow‑up.
Independent suite health at head
Re‑run locally with correct workspace resolution — matches the PR body's counts:
- core scheduler/task/outbox — 195/195
- ChannelBase — 495/495
- CLI serve delivery (IPC / dispatcher / supervisor / routes / worker / loop‑controller) — 256/256
Session.test.ts— 370/370 (with the finding‑Where is the config saved? #2 fix; 368 unchanged today)
Recommendation
Blocker #1 is genuinely resolved — nice work moving the dispatcher behind loadServeRuntimeModules‑style lazy loading. Blocker #2 is the one thing I'd still fix before merge: it silently corrupts the delivered message on a common retry path, in the exact feature this PR ships, and the fix is one small guarded block with a regression test. Once #2 lands I'm happy to re‑verify and sign off.
中文版本（点击展开）
本地真实构建与测试验证 — head 56fd99b03
维护者本地真实构建 + 真实测试验证（非纯代码走读）。我在 PR head 上建立了隔离 worktree，并把工作区依赖解析到本 PR 自己的源码，然后驱动真实的投递代码路径。
结论 — 我第 2 轮的两个阻断项一好一坏：
| # | 第 2 轮阻断项 | 现状 | 验证方式 |
|---|---|---|---|
| 1 | serve 快路径打包把 ~5.7 MB core 静态拉入 | ✅ 已修复 | 真实打包 + closure 门禁通过；esbuild metafile；A/B 对照 |
| 2 | 定时投递在流中途 retry/fallback 时重复文本 | ❌ 仍存在 | 真实测试复现；已给出一处小修复 |
此外，早先一个次要项（worker 静默丢弃 guard 失败的投递）现已修复；另有一个轻微缺口仍在。
✅ 问题 #1 — serve 快路径打包 closure：已修复并确认
dispatcher 现在在模块作用域是 type‑only 导入（run-qwen-serve.ts:121），实际通过 await import(...)（约 :5069）在监听后路径加载。我跑了真实门禁：npm run check:serve-fast-path-bundle → “closure check passed”（exit 0）。metafile 确认 dispatcher 分块只经由 dynamic-import 可达，core 不在快路径闭包内。
A/B 对照：把导入改回静态重新构建后，门禁失败并指出一个 5.78 MB 分块被拉入监听前路径（Core shell tool runtime = packages/core/src/tools/shell.ts + glob + @iarna/toml + chokidar + fzf）——正是我第 2 轮描述的回归，而当前 head 已避免它。（改动已还原；CI Serve A/B 绿。）
❌ 问题 #2 — 流中途 retry/model fallback 时投递文本重复（仍未解决）
与第 2 轮阻断项 #2 相同，投递累加块与 5daa06c 逐字节一致。cron 投递循环手工累加 turnAnswer += part.text，在 RETRY/MODEL_FALLBACK 分支只重置了 functionCalls，没有重置 turnAnswer（Session.ts:4487‑4497）。但一次“全新重启”式 retry（限流/传输错误）会把整段答案从头重新流式输出。StreamEventType.RETRY 的文档明确要求消费方“丢弃失败那次的部分内容”，geminiChat.ts 在 yield RETRY 前还调用了 popPendingPartialAssistantTurn()；core 的 Turn.run() 也正因如此在 retry 时重置 pendingToolCalls——投递循环对工具调用做了，却漏了文本。结果：投递出的 Channel 消息把 retry 前的残片拼在最终答案前面。
我在既有投递测试骨架中注入了一次“全新重启”RETRY（以及一次 MODEL_FALLBACK），断言正确的投递文本。在 PR head 上两者都失败：RETRY → 投递 "daily daily result"（应为 "daily result"）；MODEL_FALLBACK → 投递 "partial final answer"（应为 "final answer"）。
对照修复：在非 continuation 的 retry/fallback 时重置 turnAnswer，两条用例即通过，且整套 Session.test.ts 保持绿（370/370）。补丁见上方英文 diff。
范围说明：Todo‑Stop‑Guard 路径不受影响（它重新读取 getLastModelMessageText()）；受影响的是无 hook 定时任务的默认单答案投递路径。
第 2 轮次要项现状
- ✅ 已修复：worker 现在把 guard/永久性主动投递失败快速判为
channel_delivery_invalid（daemon-worker.ts:1097‑1118），不再静默拖到 30s 超时。 ⚠️ 仍在（轻微）：enqueueScheduledDelivery抛错（如答案超过MAX_TEXT_LENGTH）仅debugLogger.error（Session.ts:4639‑4644）→ 静默不投递、无用户可见提示。低优先级，建议后续跟进。
各测试套件在 head 的独立结果
core 调度/任务/outbox 195/195；ChannelBase 495/495；CLI serve 投递（IPC/dispatcher/supervisor/routes/worker/loop‑controller）256/256；Session.test.ts 370/370（含 #2 修复）。与 PR 描述的数字一致。
建议
阻断项 #1 确已解决。#2 是我认为合并前仍应修复的一项：它会在常见的 retry 路径上静默损坏投递内容，且正好发生在本 PR 交付的功能里，修复只是一小块带 guard 的代码 + 一个回归测试。#2 落地后我乐意复验并放行。
Verification method: isolated worktree at 56fd99b03; @qwen-code/* resolved to PR source; real esbuild bundle for the closure gate + A/B; real vitest runs driving the actual delivery loop. Screenshots are unmodified terminal captures.
|
@wenshao The current head
Focused local verification: Session 371/371; core scheduler/task/outbox 198/198; typecheck and lint pass. When convenient, could you re-verify the current head? |
Maintainer Local Verification ReportEnvironment: macOS (darwin), Node.js v22.22.2, branch Build✅ Unit Tests (2,628 tests, all passed)
Type Check & Lint✅ CI Status✅ Local E2E Verification1. Daemon capability advertisement Started 2. Delivery target validation Invalid target types are rejected at the route level: {
"error": "`delivery.target` requires type `user` or `chat` and a non-empty string id",
"code": "invalid_delivery"
}3. Outbox lifecycle (enqueue → claim → delivered) Exercised the 4. Security: POSIX permissions Outbox directory created with Limitations
SummaryAll automated checks pass. The daemon correctly advertises 🇨🇳 中文验证报告（点击展开）维护者本地验证报告环境： macOS (darwin), Node.js v22.22.2, 分支 构建✅ 单元测试（2,628 个测试，全部通过）
类型检查 & 代码规范✅ CI 状态✅ 本地 E2E 验证1. Daemon 能力广播 使用隔离的 2. 投递目标校验 非法目标类型在路由层被拒绝： {
"error": "`delivery.target` requires type `user` or `chat` and a non-empty string id",
"code": "invalid_delivery"
}3. Outbox 生命周期（入队 → 认领 → 已投递） 直接对临时工作区执行 4. 安全性：POSIX 权限 Outbox 目录以 局限性
结论所有自动化检查通过。Daemon 正确广播 |
Maintainer local verification — round 4 (head
|
| # | Round‑3 finding | Status now | Fix commit |
|---|---|---|---|
| 2 | Scheduled delivery duplicates text on a mid‑stream RETRY / MODEL_FALLBACK | ✅ Fixed | 9ac3f675a |
| — | Oversized (>100k) answer throws at enqueue → silent non‑delivery | ✅ Fixed | e21b8b97d |
| 1 | Serve fast‑path bundle pulled ~5.7 MB of core (re‑confirmed) | ✅ Still fixed | (unchanged) |
1) Retry duplication — fixed and load-bearing
9ac3f675a resets turnAnswer when a fresh RETRY (isContinuation !== true) or a MODEL_FALLBACK restarts the cron turn, while preserving the accumulated text for a continuation retry — exactly the boundary the corruption needed.
I ran the author's new parametrized regression as a controlled A/B: revert only the 8‑line guard, everything else identical.
- Reverted (control): a fresh restart re‑streams the whole answer, so the delivered text duplicates →
"daily daily result"and"partial final answer"(2 FAIL). This reproduces my round‑3 diagnosis byte‑for‑byte. - PR head: all 3 cases pass; the
continuation retrycase is correctly unaffected in both builds.
2) Oversized delivery text — fixed and load-bearing
e21b8b97d normalizes the answer at enqueue: it truncates over the 100k outbox bound, drops a dangling high surrogate so a UTF‑16 pair is never split, appends an explicit truncation marker, and — importantly — runs the idempotency (sameEnqueue) check against the normalized input so a repeated oversized enqueue still matches.
A/B: with normalization reverted, a >100k answer fails isValidRecord and throws Invalid scheduled delivery enqueue input. (2 FAIL) — which on the real path is a silent non‑delivery (the enqueue call only debug‑logs on throw). With the fix present, the result is truncated ≤100k, surrogate‑safe, and idempotent (3 PASS).
3) Bundle closure — still fixed
npm run check:serve-fast-path-bundle (real clean → build → DEV bundle → esbuild-metafile closure walk) passes at this head. run-qwen-serve.ts:121 keeps the dispatcher as import type (erased) and loads it at runtime via await import('./scheduled-delivery-dispatcher.js') (:5069); the gate skips dynamic-import edges, so the dispatcher's heavy transitive closure stays out of the serve fast‑path bundle.
Suites (all green at head)
- CLI
Session.test.ts— 371/371 - core
cronScheduler+cronTasksFile+scheduled-delivery-outbox— 198/198 - CLI serve/channel delivery (
ipc·dispatcher·supervisor·routes/scheduled-tasks·daemon-worker·durable-loop-controller) — 256/256 ChannelBase— 495/495; DingTalk adapter 72/72; Feishu adapter 76/76typecheck(core + cli) — pass
One remaining minor (non-blocking)
enqueueScheduledDelivery failures in Session.ts (:4647) are still only debugLogger.error — no user‑visible diagnostic. With oversized text now handled, the realistic remaining triggers (e.g. outbox full at MAX_RECORDS=200, lock contention) are edge cases, so this is an observability nit, not a blocker. Optional follow‑up.
🇨🇳 中文版本（点击展开）
维护者本地验证 —— 第 4 轮（head e21b8b97d）
在隔离 worktree 中从源码构建 PR（所有 @qwen-code/* 解析到本 PR 自身的包），并真实驱动代码路径复验——不是重新读代码。本轮针对我第 3 轮验证（56fd99b0）后仍未解决的两个问题。
结论：两个遗留问题均已解决。从验证角度看，已具备合并条件。
| # | 第 3 轮问题 | 当前状态 | 修复 commit |
|---|---|---|---|
| 2 | 中途 RETRY / MODEL_FALLBACK 时投递文本重复 | ✅ 已修复 | 9ac3f675a |
| — | 超长（>100k）回答在 enqueue 时抛错 → 静默不投递 | ✅ 已修复 | e21b8b97d |
| 1 | serve 快路径 bundle 拉入约 5.7 MB core（复查） | ✅ 仍然修复 | （未变） |
1）重试重复 —— 已修复，且修复是"承重的"。 9ac3f675a 在全新 RETRY（isContinuation !== true）或 MODEL_FALLBACK 重启 cron turn 时重置 turnAnswer，同时保留续传重试已累积的文本——正是重复所需的边界。我把作者新增的参数化回归测试当作受控 A/B：只回退那 8 行守卫，其余完全一致。回退后（对照组）全新重启会重放整段回答，投递文本重复为 "daily daily result" 和 "partial final answer"（2 失败），逐字复现我第 3 轮的诊断；PR head 下 3 个用例全过，且续传重试在两种构建下都正确不受影响。
2）超长投递文本 —— 已修复，且承重。 e21b8b97d 在 enqueue 时归一化回答：超过 100k 边界截断、丢弃悬空的高代理位以免拆断 UTF‑16 代理对、追加显式截断标记，并且——关键点——幂等（sameEnqueue）判定用的是归一化后的输入，因此重复的超长 enqueue 仍能匹配。A/B：回退归一化后，>100k 回答无法通过 isValidRecord 校验、抛 Invalid scheduled delivery enqueue input.（2 失败）——在真实路径上这是静默不投递（enqueue 抛错仅 debug 日志）；修复在场时结果被截断到 ≤100k、代理位安全、且幂等（3 通过）。
3）Bundle 闭包 —— 仍然修复。 npm run check:serve-fast-path-bundle（真实 clean → build → DEV bundle → esbuild metafile 闭包遍历）在此 head 通过。run-qwen-serve.ts:121 将 dispatcher 保持为 import type（编译期擦除），运行时经 await import('./scheduled-delivery-dispatcher.js')（:5069）加载；gate 跳过 dynamic-import 边，故 dispatcher 的重型传递闭包不进入 serve 快路径 bundle。
测试套件（head 处全绿）： CLI Session.test.ts 371/371；core cronScheduler+cronTasksFile+scheduled-delivery-outbox 198/198；CLI serve/channel 投递（ipc·dispatcher·supervisor·routes·worker·loop-controller）256/256；ChannelBase 495/495；DingTalk 72/72；Feishu 76/76；typecheck（core+cli）通过。
一个遗留小项（不阻塞）： Session.ts:4647 中 enqueueScheduledDelivery 失败仍仅 debugLogger.error，无用户可见诊断。超长文本已处理后，现实中剩余触发点（如 MAX_RECORDS=200 满、锁竞争）属边缘情况，故这是可观测性小瑕疵而非阻塞项，可作为可选后续。
|
@wenshao Thanks for the round-4 verification. Since all blocking findings are resolved on |
BZ-D
left a comment
BZ-D
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: reverse audit (Step 5) — focused review with 10 agents (3 whole-diff + 7 chunk); whole-diff agents read all 24 chunks; PR previously reviewed through 4 maintainer rounds by @wenshao. Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent was launched with it. Not reviewed: chunk 1 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 2 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 3 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 4 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 5 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 6 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 7 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 8 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 9 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 10 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 11 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 13 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 16 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 17 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 20 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 22 — its prompt was built, but no agent was launched with it. Not reviewed: chunk 24 — its prompt was built, but no agent was launched with it. Not reviewed: Test coverage matrix (whole-diff) — its prompt was built, but no agent was launched with it. Not reviewed: Agent 1b: Removed-behavior audit — its prompt was built, but no agent was launched with it. Not reviewed: reverse audit — no auditor ran (Step 5 builds its prompt with agent-prompt --role reverse-audit; none was recorded, so the pass that looks for what Step 3 missed was skipped).
— qwen via Qwen Code /review
Code Review Report — PR #7153 (head
|
| # | Finding | Verdict | Evidence |
|---|---|---|---|
| 1 | [CI] run-qwen-serve.ts static import of dispatcher pulls core chunk into serve fast path |
Fixed | run-qwen-serve.ts:121 is now import type (type-only); :5069 uses await import('./scheduled-delivery-dispatcher.js') inside loadServeRuntimeModules() |
| 2 | [correctness] turnAnswer not reset on RETRY/MODEL_FALLBACK, causing duplicated delivery text |
Fixed | Session.ts:4503 resets turnAnswer = '' for non-continuation RETRY/MODEL_FALLBACK; preserves continuation retries (isContinuation === true); regression test covers fresh retry, model fallback, and continuation |
| 3 | [robustness] >100k-char answers silently drop delivery | Fixed | scheduled-delivery-outbox.ts:256-265 normalizeDeliveryText() truncates with TRUNCATED_TEXT_SUFFIX marker; handles surrogate pairs at boundary |
| 4 | [minor] Malformed channel_delivery IPC silently dropped |
Addressed | daemon-worker.ts now sends channel_delivery_invalid for permanent errors via classifyChannelDeliveryError(); expiry and queue-full also fail-fast |
| 5 | [minor] threadId validation inconsistent across layers |
Resolved | threadId removed from delivery contract entirely; CronTaskChannelTarget is only {type, id}; parseDeliveryField trims and validates channelName/target.id consistently with isValidDelivery in cronTasksFile.ts |
Findings 6-8 (chunk-level duplicate retry, deliveryId minute granularity, PR template) are minor/follow-up and not blocking.
Review Agents — Results
10 review agents were launched (3 whole-diff + 7 chunk territory agents), covering the key production source code and cross-file dependencies. The whole-diff agents read all 24 diff chunks.
| Agent | Scope | Findings |
|---|---|---|
| Agent 0 (Issue Fidelity) | Full diff + #7152 acceptance criteria | 0 — All 14 acceptance criteria met or explicitly scoped out with maintainer acknowledgement |
| Agent 7 (Build & Test) | Build + test + test-efficacy | 0 — All 17 workspaces built; 12 test failures in packages/cli are all in files NOT touched by this PR (Chinese-locale git output, timing-sensitive UI assertions); test-efficacy: 0 unreachable, 0 inert, 7/19 test files actively gate the PR's behavior |
| Agent 1c (Cross-file tracer) | Full diff + cross-file deps | 0 — Every new type/field/method has matching producers and consumers; delivery pipeline fully wired |
| Chunk 12 | daemon-worker.ts |
0 — Follows established webhook-task patterns; tests cover async ordering, rejection, redaction, foreign error classification |
| Chunk 14 | durable-loop-controller.ts, capabilities.ts |
0 — scheduledTaskChannelDeliveryAvailable toggle is both set and read (not a dead switch) |
| Chunk 15 | channel-delivery-ipc.ts |
0 — Type guards thorough; test coverage comprehensive; no prototype pollution path |
| Chunk 18 | scheduled-tasks.ts routes |
0 — Validation consistent with core layer; DELETE guard correctly distinguishes owned vs. shared sessions |
| Chunk 19 | run-qwen-serve.ts, scheduled-delivery-dispatcher.ts |
0 — Dynamic import correct; shutdown sequence proper; retry math verified |
| Chunk 21 | cronScheduler.ts, cronTasksFile.ts |
0 — Race-condition fix verified; validation matches all test cases; new fields consistently populated |
| Chunk 23 | scheduled-delivery-outbox.ts |
0 — Proper locking (in-process mutex + cross-process file lock); symlink protection; 0o700/0o600 permissions |
CI Status
CI is green (16 checks: 7 passed, 9 skipped). Skipped checks include Integration Tests (CLI, No Sandbox) and macOS/Windows test legs — Agent 7's local build and test covered the affected packages, all passing except 12 pre-existing failures in untouched files.
Security Assessment
- Admission is fail-closed and workspace-exact with a 7-day freshness window
- DM routing uses observed conversation
chatId, never user id substitution - Credentials redacted at worker diagnostic boundary and before persisting outbox errors
channel_deliveryIPC stays internal (no webhook, no model tool)- Outbox directory
0o700, files0o600; symlink protection vialstat+noFollow: true - No prototype pollution path through
parseDeliveryField(Object.keys().every()whitelist)
Verdict
No Critical or Suggestion findings. All existing blockers are fixed. All review agents reported zero findings. Build succeeds, all PR-touched tests pass. The PR is mergeable.
— qwen via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at e21b8b97d1e41f0608e3cae5240041b897928977.
No new Critical or Suggestion finding was confirmed on the current head. I re-checked the previously reported blockers: the serve fast-path import is dynamic, fresh retry/model fallback resets the captured delivery answer while continuation retries preserve it, and oversized answers are bounded before outbox validation/idempotency comparison.
Maintainer escalation is still required by this repository's core-infrastructure gate. This is an external feat spanning 47 files and about 3,053 production diff lines across core services and multiple packages. It is not size-hard-blocked because it is not a refactor, but it exceeds the gate's bounded small-scope/100%-confidence path and the 1,000+ production-line advisory threshold.
The downstream chain I verified is: scheduled-task REST create/update/read/delete -> durable task validation/persistence -> cron scheduling/session ownership -> terminal-answer capture -> workspace-private outbox -> daemon dispatcher -> exact-workspace worker manager/group/supervisor -> delivery IPC -> daemon worker -> ChannelBase.deliverProactive() -> DingTalk/Feishu/Telegram/WeCom adapter behavior. No cross-workspace fallback was found.
Validation evidence:
- GitHub presubmit: all current checks passing; 44 checks inspected (platform/integration and automation legs are skipped by workflow routing).
- Focused local tests after building the PR's channel-base dependency: core 198; CLI scheduled-delivery/session/route/worker paths 697; ChannelBase 495; DingTalk 72; Feishu 76; Telegram 15; WeCom 135 — all passing.
- The review harness's widened build stopped in unchanged
packages/audio-captureon a local Node 24/node-gypENOENT; core and acp-bridge built successfully before that. This is not attributed to the PR, and the repository's Node 22 CI build/test check is green.
Verdict: COMMENT / maintainer review required; no confirmed blocker from this pass.
Head branch was pushed to by a user without write access
e21b8b9 to
f7829f1
Compare
ReviewOverviewAdds optional daemon-owned delivery of a scheduled task's final answer to one explicit I read the full diff and cross-checked the load-bearing details against the PR head: the Strengths
Findings
Scope notes
VerdictWell-engineered and thoroughly tested; the security posture (no prompt-driven routing, workspace isolation, hardened outbox, redaction) is convincingly enforced in both code and tests. Nothing blocking — finding 1 (duplicate-send window / worker-side dedup) is the one I'd most like addressed or explicitly documented before merge; the rest are follow-up material. |
|
Superseded by #7387 and the new implementation in #7388. The replacement broadens the contract from scheduled-only durable delivery to one immediate Channel delivery boundary shared by synchronous notifications, Prompt finals, and scheduled finals, while preserving the existing webhook 202 path. Closing this draft to keep review on the smaller current design. |
6 participants
Summary
Adds optional daemon-owned delivery of a scheduled task's final result to one explicit Channel destination.
deliveryis optional; omitting it preserves existing scheduled-task behavior./loopuses the same typed destination contract.Design
Routing is deterministic code, not a prompt or a model-visible send tool. The daemon resolves only the Channel Worker configured for the task's workspace and never falls back to another workspace.
Channel adapters keep scheduled delivery separate from ordinary proactive replies:
chatto group send anduserto OTO send.chattochat_idandusertoopen_id.The outbox directory is owner-only (
0700), and outbox/guard files are owner-only (0600). Existing permissive outbox files are healed under the lock, including no-op claims. Stored fields are strict and bounded.Scope boundaries
This PR intentionally does not add:
qwen channel startbehavior;Contact discovery from #7109 can be used by clients to present choices, but the delivery contract only requires the explicit typed target.
Validation
main.git diff --checkpassed.Real E2E
Using an isolated temporary daemon and the current built CLI:
deliveredin one attempt.deliveredin one attempt.Sanitized evidence is recorded in
.qwen/e2e-tests/scheduled-channel-delivery.md.Related