fix(channel): preserve source metadata on resume - #10257
Conversation
Ensure channel workers stamp source attribution when resuming routed sessions so legacy channel conversations are grouped under Channels instead of Tasks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the PR — the linked issue is real, and the resume-path angle matches how #10248 reads.
Moving on to code review. 🔍 中文说明感谢贡献！关联的 issue 是真实存在的，resume 路径这个切入点和 #10248 的现象吻合。
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe diff itself is clean — but tracing where the new fields actually go, they never reach the daemon. The worker now passes
The updated tests pass because they stub What an end-to-end fix would need: extend Test evidence (this PR's own CI)At review time the ubuntu unit suite is still running on this commit; macOS/Windows test legs were skipped, security checks passed, and nothing has failed. Note the point above stands regardless of the outcome: the focused test stubs the SDK boundary, so a green suite here does not demonstrate the metadata reaches the daemon. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查diff 本身是干净的——但追踪这些新字段的去向就会发现，它们到不了 daemon。worker 现在会把
更新后的测试之所以通过，是因为它们把 端到端修复需要：扩展 测试证据（本 PR 自己的 CI）审查时 ubuntu 单测仍在该提交上运行中；macOS/Windows 测试被跳过，安全检查通过，暂无失败。注意无论结果如何上面的结论都成立：focused test 把 SDK 边界打桩了，绿套件并不能证明元数据到达了 daemon。CI 明细见上方表格（由 finalize 工作流在 CI 结束后原地更新）。 沙箱验证可以一锤定音： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — right direction, real problem, but the change as written never reaches the daemon, so it cannot fix #10248. Stepping back: my independent take on this bug would have started the same way — make resume carry what create already carries — but tracing the layers first, because "pass it to the SDK client" and "the daemon receives it" are two different claims here. The PR satisfies the first and quietly fails the second: the SDK's restore transport whitelists the wire body, and the daemon's resume route sources attribution only from persisted creation metadata. The worker-side change is internally consistent, and the test updates honestly describe what the worker now does — but they assert a mocked boundary, so the suite goes green while the runtime behavior is unchanged. I also share the concern the deleted comment raises: the old code said deliberately that restore never re-stamps attribution, so this is a semantics change that deserves a maintainer's sign-off even after the plumbing is fixed. Not opposed to the approach — opposed to merging it before it does something. If the SDK transport, the resume route, and the persistence question get addressed (or a maintainer points out an existing re-attribution path I missed), a re-run here is welcome. Requesting changes — details in the review comment above. 🙏 中文说明置信度：2/5 ——方向正确、问题真实，但按当前写法这些改动到不了 daemon，因此修不好 #10248。 退一步看：我对这个 bug 的独立方案也会从同一个点出发——让 resume 携带 create 已经携带的东西——但会先追踪各层，因为"传给 SDK client"和"daemon 收到"在这里是两个不同的命题。本 PR 满足了前者，却在后者上悄悄落空：SDK 的 restore 传输层对请求体做了白名单，而 daemon 的 resume 路由只从持久化的创建元数据取来源归属。worker 侧的改动内部自洽，测试更新也如实描述了 worker 的新行为——但断言的是打桩边界，所以套件全绿而运行时行为没有任何变化。我也认同被删注释所提示的顾虑：旧代码明确写着恢复不重新标记归属，因此这是一个语义变更，即便补齐了管道，也应该先得到维护者确认。 不反对这个方案——反对的是在它还不起作用时合入。如果补齐 SDK 传输层、resume 路由和持久化问题（或者维护者指出我遗漏的既有重新归属通道），欢迎重跑 triage。 请求修改——详见上方审查评论。🙏 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@AaronZ345 Needs some rework — the resume metadata is dropped before it ever reaches the daemon: the SDK's restore transport whitelists the wire body (no source fields), and the POST /session/:id/resume route reads attribution only from persisted creation metadata. The tests pass because they stub the SDK boundary, so the runtime behavior is unchanged and #10248 stays open. Full trace and what an end-to-end fix needs are in my comments above. 🙏
— Qwen Code · qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): Tests 87 passed — this review observed 25104 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：Tests 87 passed — this review observed 25104 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| sourceType: 'channel', | ||
| // sourceId = channel instance name (e.g. feishu-main): distinguishes | ||
| // channel instances on the daemon data plane; the channel kind |
There was a problem hiding this comment.
[Critical] The sourceType: 'channel' and optional sourceId this change now passes to DaemonSessionClient.resume never reach the daemon, so the fix is a runtime no-op and issue #10248 remains unfixed. The SDK's restore transport builds the POST /session/:id/resume body from only cwd/approvalMode (plus load-only fields) — RestoreSessionRequest in packages/sdk-typescript/src/daemon/DaemonClient.ts does not declare source fields at all, so both are stripped before serialization. Even if they arrived, the daemon's resume route (restoreSessionHandler in packages/cli/src/serve/routes/session.ts) parses no source fields from the body; it re-seeds attribution only from persisted creation metadata, which is empty for exactly the legacy sessions this PR targets. A resumed DingTalk conversation created before source attribution existed therefore still carries no sourceType, and the web shell sidebar still lists it under Tasks. The updated unit tests pass anyway because they mock DaemonSessionClient.resume and assert only the factory's call arguments — they pin the very seam where the chain breaks, and the as unknown as DaemonSdkLike cast against the locally widened interface hides the mismatch at compile time. The issue's own triage prescribed both halves ("resume branch of createDaemonSessionFactory + daemon resume endpoint"); this PR implements the first only.
Witness — probe driving the built SDK with a stubbed fetch, passing exactly what the factory now passes on resume:
POST /session/legacy-session-1/resume BODY={"cwd":"/workspace"}
POST /session BODY={"cwd":"/workspace","sessionScope":"thread","sourceType":"channel","sourceId":"dingtalk-main"}
The resume arm drops both fields while the create arm carries them; patching restoreSession to serialize them flips the resume body to {"cwd":"/workspace","sourceType":"channel","sourceId":"dingtalk-main"}.
Carry the metadata end-to-end: add sourceType?/sourceId? to RestoreSessionRequest and serialize them in restoreSession's body (mirroring createOrAttachSession, including the session_source_metadata capability pre-flight), then parse and apply them in restoreSessionHandler — the bridge restore path already accepts req.sourceType/req.sourceId via sessionSourceRequestMeta. Alternatively, if resume re-attribution is intentionally out of scope, revert the resume-branch fields and fix #10248 at the layer that actually classifies/lists sessions.
Fix acceptance: add an SDK test asserting the POST /session/:id/resume body carries sourceType/sourceId when supplied, plus a serve-route test asserting a resumed unattributed session reports sourceType: 'channel' — then remove the new serialization/route wiring and confirm both tests go red. The current factory tests stay green with the transport gap intact, so they pin nothing about this fix.
中文说明
这个改动现在传给 DaemonSessionClient.resume 的 sourceType: 'channel' 和可选 sourceId 永远不会到达 daemon，因此该修复在运行时是空操作，issue #10248 仍未被修复。SDK 的 restore 传输层构造 POST /session/:id/resume 请求体时只包含 cwd/approvalMode（以及仅 load 使用的字段）——packages/sdk-typescript/src/daemon/DaemonClient.ts 中的 RestoreSessionRequest 根本没有声明 source 字段，所以两个字段在序列化之前就被丢弃了。即使它们到达服务端，daemon 的 resume 路由（packages/cli/src/serve/routes/session.ts 中的 restoreSessionHandler）也不会从请求体中解析任何 source 字段；它只用持久化的创建元数据重新填充来源归属，而这正是本 PR 针对的遗留会话所缺失的（为空）。因此，在来源归属能力出现之前创建的钉钉会话被恢复后仍然没有 sourceType，web shell 侧边栏仍会把它列在 Tasks 下。更新后的单测之所以照样通过，是因为它们 mock 了 DaemonSessionClient.resume，只断言工厂的调用参数——恰好钉在链路断裂的接缝上，而 as unknown as DaemonSdkLike 强制转换配合本地加宽的接口，让这一类型不匹配在编译期被隐藏。issue 的 triage 同时指出了两半（"createDaemonSessionFactory 的 resume 分支 + daemon resume endpoint"）；本 PR 只实现了前半部分。
证据——对构建后的 SDK 打桩 fetch 的探针，传入与工厂现在 resume 路径完全相同的参数：
POST /session/legacy-session-1/resume BODY={"cwd":"/workspace"}
POST /session BODY={"cwd":"/workspace","sessionScope":"thread","sourceType":"channel","sourceId":"dingtalk-main"}
resume 分支丢弃了两个字段，而 create 分支正常携带；将 restoreSession 补丁为序列化这两个字段后，resume 请求体翻转为 {"cwd":"/workspace","sourceType":"channel","sourceId":"dingtalk-main"}。
修复建议：将元数据端到端透传——在 RestoreSessionRequest 中增加 sourceType?/sourceId?，并在 restoreSession 的请求体中序列化它们（参照 createOrAttachSession，包括 session_source_metadata capability 预检），然后在 restoreSessionHandler 中解析并应用——bridge 的 restore 路径已经通过 sessionSourceRequestMeta 接受 req.sourceType/req.sourceId。或者，如果 resume 重新归属有意不在本次范围内，请撤销 resume 分支的这两个字段，改在实际负责会话分类/列表的层修复 #10248。
修复验收：新增一个 SDK 测试，断言提供 source 字段时 POST /session/:id/resume 请求体携带 sourceType/sourceId；再加一个 serve 路由测试，断言恢复一个无归属会话后其 sourceType 为 'channel'——然后移除新增的序列化/路由接线，确认两个测试都变红。当前的工厂测试在传输缺口存在时仍然保持绿色，因此对本修复没有任何钉住作用。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| // sourceId = channel instance name (e.g. feishu-main): distinguishes | ||
| // channel instances on the daemon data plane; the channel kind | ||
| // (dingtalk/feishu) is derivable from the name via the channel config. | ||
| ...(req.sourceId ? { sourceId: req.sourceId } : {}), |
There was a problem hiding this comment.
[Suggestion] On the resume branch, req.sourceId is never populated by any production caller, so this conditional spread only ever fires in tests. The only path reaching the factory's resume branch is SessionRouter → DaemonChannelBridge.loadSession (packages/channels/base/src/DaemonChannelBridge.ts), which deliberately omits options.sourceId — behavior pinned by its own test "forwards sourceId to the session factory only for new sessions" and documented "(new sessions only)" — and SessionRouter passes sourceId exclusively to bridge.newSession. A repo-wide sweep at this commit found 0 of 2 production resume call sites carrying sourceId. The new test "stamps channel sourceId on created and resumed sessions" therefore asserts a factory input shape no production code produces. Once the transport gap above is fixed, resumed sessions would carry sourceType: 'channel' without per-instance sourceId, so the daemon session-list sourceId filter still excludes them, and the green test codifies an impossible production contract. Either forward sourceId through the resume path in channel-base (updating the pinned bridge test) so the resumed shape is real, or reduce the resume request/interface/test to what production actually sends (sourceType only).
Fix acceptance: whichever direction is chosen, update DaemonChannelBridge.test.ts to assert the chosen contract, and confirm removing the forwarding turns the test red.
中文说明
在 resume 分支上，没有任何生产调用方会填充 req.sourceId，所以这个条件展开只会在测试中触发。到达工厂 resume 分支的唯一路径是 SessionRouter → DaemonChannelBridge.loadSession（packages/channels/base/src/DaemonChannelBridge.ts），它刻意不传 options.sourceId——这一行为被其自身测试 "forwards sourceId to the session factory only for new sessions" 钉住，且文档注明 "(new sessions only)"——并且 SessionRouter 只把 sourceId 传给 bridge.newSession。在本提交上全仓库扫描发现：2 个生产 resume 调用点中 0 个携带 sourceId。因此新测试 "stamps channel sourceId on created and resumed sessions" 断言的是生产代码永远不会产生的输入形状。一旦上面的传输缺口被修复，恢复的会话将携带 sourceType: 'channel' 但没有实例级 sourceId，daemon 会话列表的 sourceId 过滤仍会把它们排除在外，而绿色的测试会把一个不可能在生产中出现的契约固化下来。建议二选一：在 channel-base 的 resume 路径中透传 sourceId（同步更新被钉住的 bridge 测试），使恢复路径的形状真实存在；或者把 resume 请求/接口/测试缩减为生产实际发送的内容（仅 sourceType）。
修复验收：无论选择哪个方向，请更新 DaemonChannelBridge.test.ts 以断言所选契约，并确认移除转发逻辑后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Propagate channel source attribution through the SDK restore transport and daemon resume route so resumed legacy channel sessions classify correctly. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/serve/server.test.ts — no such file or directory; Tests 1189 passed — this review observed 1082, 25105, 1668, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 70 passed — this review observed 1082, 25105, 1668, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 89 passed — this review observed 1082, 25105, 1668, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/serve/server.test.ts — no such file or directory; Tests 1189 passed — this review observed 1082, 25105, 1668, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 70 passed — this review observed 1082, 25105, 1668, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 89 passed — this review observed 1082, 25105, 1668, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| sessionId, | ||
| sessionScope: this.options.sessionScope ?? 'thread', | ||
| ...(options?.approvalMode ? { approvalMode: options.approvalMode } : {}), | ||
| ...(options?.sourceId ? { sourceId: options.sourceId } : {}), |
There was a problem hiding this comment.
[Suggestion] R1-2: This round added the loadSession sourceId hop and flipped the pinned test, but round 1's concern still stands — the chain terminates one hop short of production. SessionRouter.sessionOptions() returns only { approvalMode }, and neither production load call site (SessionRouter.ts:287 loadOrReplaceSession, SessionRouter.ts:598 restoreSessions) passes a sourceId, while the create path does (createLiveSession passes channelName as sourceId). A resumed legacy session is stamped sourceType: 'channel' — which fixes the Tasks misclassification — but carries no sourceId, while new sessions of the same channel carry sourceId: 'dingtalk-main': groupSessionsByChannelType places resumed conversations in the "Other channels" fallback group while new ones group under the named platform, and sourceId-scoped queries — the exact ?sourceType=channel&sourceId=… query the new server test exercises — match only new sessions. The updated doc comment ("restore-time attribution for legacy sessions resumed through a channel") likewise overpromises. Pass the channel name as sourceId at both SessionRouter load sites (e.g. { ...this.sessionOptions(channelName), sourceId: channelName }); or, if restore-time sourceId is deliberately deferred, scope the doc comment and the renamed bridge test back to what production actually sends. Fix acceptance: add a SessionRouter.test.ts case asserting bridge.loadSession receives sourceId: <channelName> on restore, and confirm removing the router wiring turns it red — the updated DaemonChannelBridge.test.ts case pins only the bridge hop and stays green regardless of the router.
中文说明
[Suggestion] R1-2：本轮新增了 loadSession 的 sourceId 转发并翻转了对应测试，但第一轮的问题依然存在——链路在距离生产一步之遥的地方断开。SessionRouter.sessionOptions() 只返回 { approvalMode }，两个生产 load 调用点（SessionRouter.ts:287 的 loadOrReplaceSession、SessionRouter.ts:598 的 restoreSessions）都不传 sourceId，而 create 路径会传（createLiveSession 把 channelName 作为 sourceId 传入）。恢复的遗留会话会被打上 sourceType: 'channel'（Tasks 误分类因此被修复），但没有 sourceId，而同一 channel 新建的会话携带 sourceId: 'dingtalk-main'：groupSessionsByChannelType 会把恢复的会话放进"其他渠道"兜底分组，而按 sourceId 过滤的查询（正是新 server 测试所断言的 ?sourceType=channel&sourceId=… 查询）只会命中新会话。更新后的文档注释（"通过 channel 恢复的遗留会话在恢复时归属"）同样言过其实。请在 SessionRouter 的两个 load 调用点把 channel 名称作为 sourceId 传入（例如 { ...this.sessionOptions(channelName), sourceId: channelName }）；或者，如果恢复时的 sourceId 有意暂缓，请把文档注释和改名后的 bridge 测试收敛回生产实际发送的形状。修复验收：新增一个 SessionRouter.test.ts 用例，断言恢复时 bridge.loadSession 收到 sourceId: <channelName>，并确认移除 router 接线后该测试变红——更新后的 DaemonChannelBridge.test.ts 用例只钉住 bridge 这一跳，无论 router 是否传值都保持绿色。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const restoreRequestMetadata = { | ||
| ...restoreMetadata, | ||
| ...(restoreSource ?? {}), | ||
| }; |
There was a problem hiding this comment.
[Suggestion] The caller-supplied restore source unconditionally overrides the session's persisted creation attribution. restoreSource is spread last, so any daemon-token client can POST /session/:id/resume with {sourceType: 'channel', sourceId: 'x'} against an already-attributed session (scheduled_task, web_shell, or a daemon-owned Live Voice session default/realtime_voice:… — the validation above checks only the request's own values, never against the stored ones) and re-stamp the cold-restored live entry via createSessionEntry. For a Live Voice source this makes isCompatibleLiveSessionSource false so live surfaces drop the session, and it merges experimental.cron: false plus the channel display-text projection into the restored child config. The warm/raced-entry paths refuse the same overwrite and return the existing source, so the identical request behaves differently depending on whether the entry is live — and the JSDoc this diff adds says "Restore-time attribution for legacy/unattributed sessions". Persisted metadata is safe (recordSessionSource refuses rewrites); the damage is per live entry. Apply the restore source only when the stored metadata carries none:
| const restoreRequestMetadata = { | |
| ...restoreMetadata, | |
| ...(restoreSource ?? {}), | |
| }; | |
| const restoreRequestMetadata = { | |
| ...restoreMetadata, | |
| ...(restoreMetadata.sourceType === undefined | |
| ? (restoreSource ?? {}) | |
| : {}), | |
| }; |
Legacy unattributed sessions (this PR's target, metadata {}) still get attributed; already-attributed sessions keep their creation source. Fix acceptance: add a restore test where stored creation metadata { sourceType: 'channel', sourceId: 'a' } plus request sourceId: 'b' must forward sourceId: 'a' to the bridge, and confirm removing the guard turns it red.
中文说明
[Suggestion] 调用方提供的 restore source 会无条件覆盖会话持久化的创建归属。restoreSource 被展开在最后，因此任何持有 daemon token 的客户端都可以对一个已有归属的会话（scheduled_task、web_shell，或 daemon 自有的 Live Voice 会话 default/realtime_voice:…——上面的校验只检查请求自身的取值，从不与已存储的来源比较）POST /session/:id/resume 并携带 {sourceType: 'channel', sourceId: 'x'}，从而通过 createSessionEntry 重新改写冷恢复的 live entry。对 Live Voice 来源而言，这会使 isCompatibleLiveSessionSource 为 false，导致 live 界面丢弃该会话，同时还会把 experimental.cron: false 和 channel 展示文本投影合并进恢复出的子进程配置。而 warm/raced-entry 路径拒绝同样的覆盖并返回已有来源——同一个请求在 entry 是否存活时行为不一致，而本 diff 新增的 JSDoc 写的是"面向遗留/无归属会话的恢复时归属"。持久化元数据本身是安全的（recordSessionSource 拒绝覆写）；损害仅限于 live entry。请仅在存储元数据没有来源时才应用 restore source（见上方 suggestion）。遗留的无归属会话（本 PR 的目标，元数据为 {}）仍然会被归属；已有归属的会话保留其创建来源。修复验收：新增一个 restore 测试——存储的创建元数据为 { sourceType: 'channel', sourceId: 'a' }，请求携带 sourceId: 'b' 时必须向 bridge 转发 sourceId: 'a'，并确认移除该守卫后测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (req.sourceType !== undefined || req.sourceId !== undefined) { | ||
| await this.requireCapability('session_source_metadata'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The new requireCapability('session_source_metadata') gate has no capability-missing rejection test, unlike its create-side and list-side siblings, which each have one (DaemonClient.test.ts "rejects source metadata before creating against an old daemon", "rejects source filtering before listing against an old daemon"); the new SDK test covers only the happy path where the daemon advertises the feature. If a future refactor deletes or swallows this guard, CI stays green and a new SDK sends sourceType/sourceId to an older daemon that silently ignores them — the exact silent-no-op shape round 1 caught for this PR — instead of failing with DaemonCapabilityMissingError. Mirror the sibling test: capabilities response without session_source_metadata, DaemonSessionClient.resume(client, 's-1', { workspaceCwd, sourceType: 'channel' }) rejects with DaemonCapabilityMissingError, and no /session/s-1/resume request is issued. Fix acceptance: that new test must go red if this three-line guard is removed.
中文说明
[Suggestion] 新增的 requireCapability('session_source_metadata') 门禁没有 capability 缺失时的拒绝测试，而 create 侧和 list 侧的同类门禁各有一个（DaemonClient.test.ts 的 "rejects source metadata before creating against an old daemon"、"rejects source filtering before listing against an old daemon"）；新增的 SDK 测试只覆盖 daemon 声明支持该特性的顺利路径。如果未来重构删除或吞掉这个守卫，CI 仍然绿色，新版 SDK 会向旧版 daemon 发送 sourceType/sourceId 并被静默忽略——正是第一轮在本 PR 上抓到的那种静默空操作——而不是以 DaemonCapabilityMissingError 失败。请参照同类测试补齐：capabilities 响应不含 session_source_metadata 时，DaemonSessionClient.resume(client, 's-1', { workspaceCwd, sourceType: 'channel' }) 以 DaemonCapabilityMissingError 拒绝，且不发出 /session/s-1/resume 请求。修复验收：移除这三行守卫后该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const hasRestoreSource = | ||
| body['sourceType'] !== undefined || body['sourceId'] !== undefined; |
There was a problem hiding this comment.
[Suggestion] The three new 400 branches below (reserved-standalone, invalid, reserved-live) and the action === 'load' forwarding branch have no tests, while the create-path sibling block is fully pinned by three rejection tests in server.test.ts; this diff adds only the resume happy path. The reserved-live check has no second layer: acp-bridge restoreSession backstops only reserved standalone sources, so this route check is the sole fence protecting the default/realtime_voice: namespace on restore. Deleting any branch — or only the load-branch ...restoreRequestMetadata spread, which is round 1's exact no-op failure mode — leaves the suite green, letting a client stamp sourceType: 'standalone' or a forged realtime_voice: source onto a restored session, or silently disabling load-time attribution. Mirror the create-path tests for /session/:id/resume and /load: {sourceType: 'standalone'} → 400 reserved_session_source; {sourceId: 'x'} alone → 400 invalid_session_source; {sourceType: 'default', sourceId: 'realtime_voice:…'} → 400 reserved_session_source; plus a /load happy path asserting bridge.loadCalls[0] carries the source. Fix acceptance: deleting any of the new branches must turn the corresponding test red.
中文说明
[Suggestion] 下方新增的三个 400 分支（保留的 standalone、非法来源、保留的 live 来源）以及 action === 'load' 的转发分支都没有测试，而 create 路径的同类代码块在 server.test.ts 中有三个拒绝测试完整钉住；本 diff 只新增了 resume 的顺利路径。保留 live 来源的检查没有第二道防线：acp-bridge 的 restoreSession 只对保留的 standalone 来源兜底，因此这个路由检查是 restore 路径上保护 default/realtime_voice: 命名空间的唯一栅栏。删除任何一个分支——哪怕只删掉 load 分支的 ...restoreRequestMetadata 展开（正是第一轮抓到的空操作失效模式）——测试套件依然绿色，客户端就能给恢复的会话打上 sourceType: 'standalone' 或伪造的 realtime_voice: 来源，或者让 load 时的归属静默失效。请参照 create 路径的测试为 /session/:id/resume 和 /load 补齐：{sourceType: 'standalone'} → 400 reserved_session_source；仅 {sourceId: 'x'} → 400 invalid_session_source；{sourceType: 'default', sourceId: 'realtime_voice:…'} → 400 reserved_session_source；另加一个 /load 顺利路径，断言 bridge.loadCalls[0] 携带来源。修复验收：删除任一新增分支后对应用例必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const hasRestoreSource = | ||
| body['sourceType'] !== undefined || body['sourceId'] !== undefined; | ||
| let restoreSource: { sourceType?: string; sourceId?: string } | undefined; | ||
| if (hasRestoreSource) { |
There was a problem hiding this comment.
[Suggestion] This ~37-line validation block is a near-verbatim copy of the creation handler's block (session.ts:2672-2703) — same three-step sequence, byte-identical error strings and codes — instead of a shared helper, which is this file's established convention (parseHistoryPageSize, parseLiveReplayMode, parseOptionalApprovalMode, the last already reused by both handlers). The hasRestoreSource gate is behaviorally a no-op: with both fields absent, the creation-style unconditional block behaves identically (parseSessionSource(undefined, undefined) returns {} and both reserved checks are false). Session-source acceptance rules now live in two places in one file (plus a third, JSON-RPC-shaped copy in acp-http/dispatch.ts): any future change — a new reserved source type, a changed error code, an extra validation step — must be edited in lockstep, and updating the create gate while missing this copy lets a client stamp a reserved source through /session/:id/resume even though create blocks it. Extract a helper following the existing parse-helper pattern (e.g. parseRequestedSessionSource(body, res) running the three checks and sending the 400s) and use it in both handlers; since the no-metadata result is {}, this handler can drop hasRestoreSource/restoreSource and keep ...(source ?? {}) unchanged. Fix acceptance: after extraction, removing either call site or mutating the shared helper's reserved-source check must turn the corresponding test red.
中文说明
[Suggestion] 这个约 37 行的校验块几乎逐字复制了 create 处理器的代码块（session.ts:2672-2703）——相同的三段式流程、逐字节相同的错误文案与错误码——而没有抽成共享 helper，而抽 helper 正是本文件的既有惯例（parseHistoryPageSize、parseLiveReplayMode、parseOptionalApprovalMode，其中最后一个已被两个处理器复用）。hasRestoreSource 门在行为上是空操作：两个字段都缺省时，create 风格的无条件代码块行为完全一致（parseSessionSource(undefined, undefined) 返回 {}，两个保留来源检查都为 false）。会话来源的准入规则现在同一文件里存于两处（外加 acp-http/dispatch.ts 里第三份 JSON-RPC 形状的副本）：未来任何变更——新增保留来源类型、修改错误码、增加校验步骤——都必须同步改两处，一旦只改了 create 的栅栏而漏掉这份副本，客户端就能在 create 已拦截的情况下通过 /session/:id/resume 打上保留来源。请按照既有 parse-helper 模式抽出辅助函数（例如 parseRequestedSessionSource(body, res)，执行三项检查并发送 400），在两个处理器中复用；由于无元数据时结果为 {}，本处理器可以去掉 hasRestoreSource/restoreSource，保持 ...(source ?? {}) 不变。修复验收：抽取后，移除任一调用点或改动共享 helper 的保留来源检查，必须让对应测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| /** Restore-time attribution for legacy/unattributed sessions. */ | ||
| sourceType?: string; |
There was a problem hiding this comment.
[Suggestion] Restore-time attribution is live-only and never persisted, so the parity this JSDoc promises holds only for the live entry. The restore path seeds the live entry and the child config but never invokes the sessionSource serve-control extension — the only writer of the durable session_source transcript record is reachable solely from doSpawn/branch/ensureDefaultSessionPersisted, never from restore. New channel sessions get durable attribution; resumed legacy ones do not. A legacy DingTalk conversation resumed today shows under Channels, but after a daemon restart (upgrade/reboot) or any live-entry loss, the persisted transcript has no session_source record, the merged listing falls back to stored (empty) metadata, and the conversation reappears under Tasks until the next channel message re-resumes it — a transient recurrence of #10248's exact symptom (channel-driven flows self-heal per message; SDK-direct restores never do). Persist the restore-time source the same way doSpawn does: after a restore settles with req.sourceType, invoke the sessionSource control extension in bridge.ts, mirroring doSpawn's source-persist block (recordSessionSource is idempotency-safe — it refuses to overwrite an existing differing attribution). Fix acceptance: a bridge test asserting loadSession/resumeSession with sourceType invokes SERVE_CONTROL_EXT_METHODS.sessionSource; removing the persistence call must turn it red.
中文说明
[Suggestion] 恢复时归属只作用于 live entry，从不持久化，因此这段 JSDoc 承诺的对等性只在 live entry 存活期间成立。restore 路径会填充 live entry 和子进程配置，但从不调用 sessionSource serve-control 扩展——持久 session_source 转录记录的唯一写入点只能从 doSpawn/branch/ensureDefaultSessionPersisted 到达，restore 路径永远到不了。新建的 channel 会话获得持久归属，恢复的遗留会话则没有。今天恢复的遗留钉钉会话会显示在 Channels 下，但 daemon 重启（升级/重启机器）或任何 live entry 丢失之后，持久化转录里没有 session_source 记录，合并列表回退到（空的）存储元数据，会话会重新出现在 Tasks 下，直到下一条 channel 消息再次恢复它——#10248 症状的短暂复发（channel 驱动的流程每条消息自愈；SDK 直接恢复则永远不会自愈）。请参照 doSpawn 的做法持久化恢复时来源：当携带 req.sourceType 的 restore 落定后，在 bridge.ts 中调用 sessionSource 控制扩展，镜像 doSpawn 的来源持久化代码块（recordSessionSource 是幂等安全的——它拒绝覆写已存在的不同归属）。修复验收：新增 bridge 测试，断言携带 sourceType 的 loadSession/resumeSession 会调用 SERVE_CONTROL_EXT_METHODS.sessionSource；移除该持久化调用后测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| : await runtime.bridge.resumeSession({ | ||
| sessionId, | ||
| workspaceCwd, | ||
| ...(clientId !== undefined ? { clientId } : {}), | ||
| ...(approvalMode !== undefined ? { approvalMode } : {}), | ||
| ...restoreMetadata, | ||
| ...restoreRequestMetadata, |
There was a problem hiding this comment.
[Suggestion] Restore-time attribution lands on cold restores only — a warm attach never backfills a missing source. The bridge's existing-entry early-return and raced-entry branch return the entry's existing source and never read req.sourceType/req.sourceId (zero reads in either warm branch). A legacy channel session (no stored source — the #10248 shape) first resumed through another surface that carries no source (e.g. the web shell) gets a live entry with no sourceType; when the channel daemon then resumes the same session id — the factory always stamps sourceType: 'channel' — the warm attach discards it, and the session keeps listing under Tasks while the channel actively chats in it, until the entry dies. This is reachable in the production configuration without any race: the channel worker runs recoveryMode: 'lazy', so startup restores only routing tables. In the bridge's existing-entry and raced-entry restore paths, apply the requested source only when the entry has none (if (!existing.sourceType && req.sourceType) { existing.sourceType = req.sourceType; existing.sourceId = req.sourceId; }), preserving the no-overwrite semantics; or document in ChannelAgentBridgeSessionOptions that restore-time attribution takes effect only on cold restore. Fix acceptance: an acp-bridge test where a source-less entry resumed with sourceType/sourceId reports the new source, and deleting the fill-if-absent branch turns it red (the round-2 resume-forwarding test cannot pin this because fakeBridge never models a pre-existing entry).
中文说明
[Suggestion] 恢复时归属只在冷恢复时生效——warm attach 从不回填缺失的来源。bridge 的 existing-entry 提前返回分支和 raced-entry 分支返回 entry 已有的来源，从不读取 req.sourceType/req.sourceId（两个 warm 分支中零次读取）。一个遗留 channel 会话（没有存储来源——正是 #10248 的形状）如果先被另一个不携带来源的入口（例如 web shell）恢复，会得到一个没有 sourceType 的 live entry；随后 channel daemon 恢复同一会话 id（工厂总是打上 sourceType: 'channel'）时，warm attach 会丢弃它，该会话在 channel 持续对话期间仍然列在 Tasks 下，直到 entry 消亡。这在生产配置下无需任何竞态即可达：channel worker 以 recoveryMode: 'lazy' 运行，启动时只恢复路由表。请在 bridge 的 existing-entry 和 raced-entry 恢复路径中，仅当 entry 没有来源时才应用请求携带的来源（if (!existing.sourceType && req.sourceType) { existing.sourceType = req.sourceType; existing.sourceId = req.sourceId; }），保留不覆写语义；或者在 ChannelAgentBridgeSessionOptions 中注明恢复时归属只在冷恢复生效。修复验收：新增 acp-bridge 测试——对无来源的 entry 携带 sourceType/sourceId 恢复后应报告新来源，删除"缺失则回填"分支后测试变红（第二轮的 resume 转发测试无法钉住这一点，因为 fakeBridge 从不模拟已存在的 entry）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Ensure restored channel sessions carry source attribution through the SDK, daemon route, and ACP bridge without overwriting existing persisted attribution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): Tests 87 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 1110 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 71 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 805 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 89 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; and 1 more.
Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 7 (6 new). Findings keep coming back to the same files: packages/sdk-typescript/src/daemon/DaemonClient.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
[Critical] R3-1 — packages/cli/src/serve/routes/session.ts:3622-3628 — Restore-time caller-supplied source metadata is admitted on the internal live-conversation runtime, and the bridge's new backfill then persists it into the legacy conversation transcript, permanently breaking the conversation-loadability classification. The merge spreads restoreSource whenever hasPersistedSource is false — which includes every unattributed legacy conversation on the Conversations runtime — and applyRestoreSourceIfMissing / the cold-restore persistSessionSource write an immutable session_source record (recordSessionSource is first-writer-wins and accepts it because currentSourceType is undefined). From then on classifyTopLevelConversationSource / readLoadableLiveConversationMetadata reject the rewritten shape (a channel source with a sourceId is neither a live shape nor a compatible legacy-standalone shape), so the restore route throws SessionNotFoundError for that id on every later attempt — the session is permanently unloadable through the daemon API, reachable only by hand-editing the JSONL. One well-formed request from any authenticated client triggers it (the SDK surface this PR extends now exposes sourceType/sourceId on restore); no reserved-source gate catches channel + sourceId. Suggested fix: keep caller-supplied restore source out of the internal runtime — e.g. ...(hasPersistedSource || isInternalWorkspaceRuntime(runtime) ? {} : restoreSource) — or reject a non-empty restoreSource with 400 when the resolved runtime is internal. Witness — probe driving the real classification code at HEAD (both legacy shapes flip): SUB BEFORE backfill loadable: {"kind":"live","persistence":"legacy",...} → SUB AFTER backfill loadable: undefined; TOP BEFORE backfill loadable: {"kind":"standalone","persistence":"legacy","metadata":{}} → TOP AFTER backfill loadable: undefined. Fix acceptance: a server.test.ts case restoring a legacy unattributed conversation on the internal runtime with a client-supplied source, asserting the bridge call carries no sourceType/sourceId; it must go red when the exclusion is removed.
中文说明
R3-1 — packages/cli/src/serve/routes/session.ts:3622-3628 — restore 阶段由调用方提供的 source 元数据在内部 live-conversation 运行时上也被接受，而 bridge 新增的 backfill 会把它持久化进遗留会话的 transcript，从而永久破坏会话可加载性分类。只要hasPersistedSource 为 false，合并逻辑就会展开 restoreSource——这覆盖了 Conversations 运行时上所有无归属的遗留会话——随后 applyRestoreSourceIfMissing / 冷恢复路径的 persistSessionSource 写入一条不可变的 session_source 记录（recordSessionSource 采用先到先得，且由于 currentSourceType 为 undefined，写入会成功）。此后 classifyTopLevelConversationSource / readLoadableLiveConversationMetadata 会拒绝被改写后的形状（带 sourceId 的 channel 来源既不是 live 形状，也不是兼容的遗留 standalone 形状），restore 路由会在之后每次尝试恢复该 id 时抛出 SessionNotFoundError——会话从此无法再通过 daemon API 加载，只能手工编辑 JSONL 才能恢复。任何一个已认证客户端发出一个格式合法的请求即可触发（本 PR 扩展的 SDK 接口现在在 restore 时暴露 sourceType/sourceId）；保留来源的校验门不会拦截 channel + sourceId。修复建议：让调用方提供的 restore 来源远离内部运行时——例如 ...(hasPersistedSource || isInternalWorkspaceRuntime(runtime) ? {} : restoreSource)——或者当解析出的运行时是内部运行时时，对非空 restoreSource 返回 400。证据——在 HEAD 上对真实分类代码的探针（两种遗留形状都发生翻转）：SUB BEFORE backfill loadable: {"kind":"live","persistence":"legacy",...} → SUB AFTER backfill loadable: undefined；TOP BEFORE backfill loadable: {"kind":"standalone","persistence":"legacy","metadata":{}} → TOP AFTER backfill loadable: undefined。修复验收：在 server.test.ts 中新增用例——在内部运行时上以调用方提供的 source 恢复一个无归属的遗留会话，断言 bridge 调用不携带 sourceType/sourceId；移除该排除逻辑时测试必须变红。 中文说明
Test Plan（非阻断）：Tests 87 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 1110 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 71 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 805 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; Tests 89 passed — this review observed 1744, 1083, 25113, 1669, 404, 229, 266, 207, 59, 2, 302, 17, 135, 88, 605, 4329, 638 passed; and 1 more。
收敛情况：第 3 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 7 条（其中 6 条首次提出）。发现反复回到同一批文件：packages/sdk-typescript/src/daemon/DaemonClient.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] R3-1 — packages/cli/src/serve/routes/session.ts:3622-3628 — Restore-time caller-supplied source metadata is admitted on the internal live-conversation runtime, and the bridge's new backfill then persists it into the legacy conversation transcript, permanently breaking the conversation-loadability classification. The merge spreads restoreSource whenever hasPersistedSource is false — which includes every unattributed legacy conversation on the Conversations runtime — and applyRestoreSourceIfMissing / the cold-restore persistSessionSource write an immutable session_source record (recordSessionSource is first-writer-wins and accepts it because currentSourceType is undefined). From then on classifyTopLevelConversationSource / readLoadableLiveConversationMetadata reject the rewritten shape (a channel source with a sourceId is neither a live shape nor a compatible legacy-standalone shape), so the restore route throws SessionNotFoundError for that id on every later attempt — the session is permanently unloadable through the daemon API, reachable only by hand-editing the JSONL. One well-formed request from any authenticated client triggers it (the SDK surface this PR extends now exposes sourceType/sourceId on restore); no reserved-source gate catches channel + sourceId. Suggested fix: keep caller-supplied restore source out of the internal runtime — e.g. ...(hasPersistedSource || isInternalWorkspaceRuntime(runtime) ? {} : restoreSource) — or reject a non-empty restoreSource with 400 when the resolved runtime is internal. Witness — probe driving the real classification code at HEAD (both legacy shapes flip): SUB BEFORE backfill loadable: {"kind":"live","persistence":"legacy",...} → SUB AFTER backfill loadable: undefined; TOP BEFORE backfill loadable: {"kind":"standalone","persistence":"legacy","metadata":{}} → TOP AFTER backfill loadable: undefined. Fix acceptance: a server.test.ts case restoring a legacy unattributed conversation on the internal runtime with a client-supplied source, asserting the bridge call carries no sourceType/sourceId; it must go red when the exclusion is removed.
中文说明
R3-1 — packages/cli/src/serve/routes/session.ts:3622-3628 — restore 阶段由调用方提供的 source 元数据在内部 live-conversation 运行时上也被接受，而 bridge 新增的 backfill 会把它持久化进遗留会话的 transcript，从而永久破坏会话可加载性分类。只要hasPersistedSource 为 false，合并逻辑就会展开 restoreSource——这覆盖了 Conversations 运行时上所有无归属的遗留会话——随后 applyRestoreSourceIfMissing / 冷恢复路径的 persistSessionSource 写入一条不可变的 session_source 记录（recordSessionSource 采用先到先得，且由于 currentSourceType 为 undefined，写入会成功）。此后 classifyTopLevelConversationSource / readLoadableLiveConversationMetadata 会拒绝被改写后的形状（带 sourceId 的 channel 来源既不是 live 形状，也不是兼容的遗留 standalone 形状），restore 路由会在之后每次尝试恢复该 id 时抛出 SessionNotFoundError——会话从此无法再通过 daemon API 加载，只能手工编辑 JSONL 才能恢复。任何一个已认证客户端发出一个格式合法的请求即可触发（本 PR 扩展的 SDK 接口现在在 restore 时暴露 sourceType/sourceId）；保留来源的校验门不会拦截 channel + sourceId。修复建议：让调用方提供的 restore 来源远离内部运行时——例如 ...(hasPersistedSource || isInternalWorkspaceRuntime(runtime) ? {} : restoreSource)——或者当解析出的运行时是内部运行时时，对非空 restoreSource 返回 400。证据——在 HEAD 上对真实分类代码的探针（两种遗留形状都发生翻转）：SUB BEFORE backfill loadable: {"kind":"live","persistence":"legacy",...} → SUB AFTER backfill loadable: undefined；TOP BEFORE backfill loadable: {"kind":"standalone","persistence":"legacy","metadata":{}} → TOP AFTER backfill loadable: undefined。修复验收：在 server.test.ts 中新增用例——在内部运行时上以调用方提供的 source 恢复一个无归属的遗留会话，断言 bridge 调用不携带 sourceType/sourceId；移除该排除逻辑时测试必须变红。 — qwen3.8-max via Qwen Code /review (v0.22.2)
| options: ChannelAgentBridgeSessionOptions, | ||
| operation: SessionOperation, | ||
| sourceId: string, |
There was a problem hiding this comment.
[Suggestion] The trailing sourceId parameter became redundant this round: sessionOptions() now always stamps sourceId: channelName, so the { ...options, sourceId } spread in createLiveSession carries the same value twice, and the positional silently shadows options.sourceId — while the sibling loadSession call sites pass options through with no override. Nothing misbehaves today: both call sites pass input.channelName, byte-identical to sessionOptions(input.channelName).sourceId. But one value now travels two channels — if the sessionOptions derivation later changes (say, to a namespaced instance id), the create path keeps the stale override and newly created sessions get different attribution than restored ones, with no error surfacing the divergence. Drop the parameter and the spread: call this.bridge.newSession(cwd, options, operation) and remove the trailing input.channelName argument at both call sites, so create and restore both take sourceId solely from sessionOptions. The existing SessionRouter.test.ts assertions that bridge.newSession receives { sourceId: 'ch' } pin the stamping — keep them green after removing the parameter, and confirm they go red if sourceId is dropped from sessionOptions instead.
中文说明
尾部的 sourceId 参数在本轮改动后变得冗余：sessionOptions() 现在总会写入 sourceId: channelName，因此 createLiveSession 中的 { ...options, sourceId } 展开重复携带了同一个值，且位置参数会静默覆盖 options.sourceId——而兄弟的 loadSession 调用点直接透传 options，没有任何覆盖。今天不会出现错误行为：两个调用点都传入 input.channelName，与 sessionOptions(input.channelName).sourceId 逐字节相同。但同一个值现在经由两条通道传递——如果将来 sessionOptions 的推导方式发生变化（例如改为带命名空间的实例 id），create 路径会继续使用过时的覆盖值，导致新建会话与恢复会话的归属不一致，而且没有任何错误能暴露这种分歧。建议删除该参数和展开：直接调用 this.bridge.newSession(cwd, options, operation)，并在两个调用点移除尾部的 input.channelName 实参，让 create 和 restore 都只从 sessionOptions 获取 sourceId。SessionRouter.test.ts 中断言 bridge.newSession 收到 { sourceId: 'ch' } 的用例钉住了这一写入行为——删除参数后它们应保持绿色；如果改为从 sessionOptions 中移除 sourceId，请确认这些测试会变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (entry.sourceType !== undefined || req.sourceType === undefined) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
[Suggestion] No test pins this no-overwrite guard, and a mutation proves the gap: dropping entry.sourceType !== undefined || from the condition survives the whole suite (all 805 tests in bridge.test.ts stay green), letting a restore silently re-attribute an entry that already carries a source. The concrete trigger the guard exists for: a session spawned live with sourceType: 'scheduled_task' whose sessionSource persistence failed (sourcePersisted=false, so the route-level persisted-metadata merge cannot protect it) is later resumed with sourceType: 'channel' — the entry is silently re-attributed and the wrong source persisted, misclassifying the session in the sidebar. The only existing "does not replace" tests guard other layers (the route merge and the spawnOrAttach single-scope case). Add a case here: spawnOrAttach with sourceType: 'scheduled_task', sourceId: 'task-1', then resumeSession with sourceType: 'channel', sourceId: 'dingtalk-main'; assert the response and getSessionSummary still report scheduled_task/task-1 and no sessionSource ext call carries the channel params — then remove the guard and confirm the new test goes red (a scratch-tree mutation flipped the entry to "sourceType": "channel" with the existing suite still green).
中文说明
这个"不覆盖已有归属"的守卫没有任何测试钉住，变异测试证明了这一缺口：从条件中删除 entry.sourceType !== undefined || 后，整个测试套件仍然全部通过（bridge.test.ts 的 805 个测试保持绿色），使得 restore 可以静默地重新归属一个已有来源的条目。该守卫存在的具体触发场景：一个以 sourceType: 'scheduled_task' 实时 spawn 的会话，其 sessionSource 持久化失败（sourcePersisted=false，因此路由层的持久化元数据合并无法保护它），之后被以 sourceType: 'channel' 恢复——该条目被静默重新归属并持久化了错误的来源，导致会话在侧边栏中被错误分类。现有的"不替换"测试只守护了其他层（路由合并和 spawnOrAttach 单 scope 场景）。建议在这里增加一个用例：先以 sourceType: 'scheduled_task', sourceId: 'task-1' 调用 spawnOrAttach，再以 sourceType: 'channel', sourceId: 'dingtalk-main' 调用 resumeSession；断言响应和 getSessionSummary 仍报告 scheduled_task/task-1，且 sessionSource ext 调用中不包含 channel 参数——然后移除该守卫，确认新测试变红（临时树变异中，条目翻转为 "sourceType": "channel"，而现有套件仍然全绿）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| } catch (err) { | ||
| writeStderrLine( | ||
| `qwen serve: source metadata for ${logContext} was not persisted ` + |
There was a problem hiding this comment.
[Suggestion] This persist-failure branch (catch → stderr → return false → response sourcePersisted: false) has zero coverage at any of the new restore call sites — every new bridge test stubs the sessionSource ext method to return { persisted: true }. This diff newly makes loadSession/resumeSession run the persist, so "restore succeeds with sourcePersisted: false when the control call fails" is a new contract of this PR, and it is untested: turning this catch into a rethrow makes restore reject whenever the ext method throws or times out (e.g. an ACP child that rejects the method), and no test fails — the degradation story the PR depends on would ship green. Add a bridge.test.ts case that stubs the sessionSource ext method to throw, then asserts loadSession/resumeSession (and the backfill variant) resolve with sourcePersisted: false and the entry still carries the live-only attribution; confirm the test goes red when the catch/return false is removed or rethrows (a scratch-tree probe flipped both cold and hot restore paths from RESTORE-DEGRADED-OK to RESTORE-REJECTED under that mutation while the existing suite stayed green).
中文说明
这个持久化失败分支（catch → stderr → return false → 响应 sourcePersisted: false）在所有新的 restore 调用点上都没有任何覆盖——所有新的 bridge 测试都把 sessionSource ext 方法 stub 为返回 { persisted: true }。本次改动使 loadSession/resumeSession 开始执行持久化，因此"控制调用失败时 restore 仍以 sourcePersisted: false 成功"是本 PR 的一份新契约，且没有测试：把这个 catch 改成重新抛出后，只要 ext 方法抛错或超时（例如某个 ACP 子进程拒绝该方法），restore 就会 reject，而没有任何测试失败——本 PR 所依赖的降级故事会在一片绿色中上线。建议在 bridge.test.ts 中增加一个用例：把 sessionSource ext 方法 stub 为抛错，然后断言 loadSession/resumeSession（以及 backfill 变体）以 sourcePersisted: false 正常 resolve，且条目仍携带仅存于内存的来源归属；确认移除 catch/return false 或改为重新抛出时该测试变红（临时树探针在该变异下把冷、热两条 restore 路径都从 RESTORE-DEGRADED-OK 翻转为 RESTORE-REJECTED，而现有套件保持绿色）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| if (!(error instanceof DaemonCapabilityMissingError)) { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R2-2: (fix-induced) The fix for R2-2 replaced the hard requireCapability gate with this try/catch graceful degrade and added the omission test, closing the reported gap — but the change introduced this rethrow branch for errors that are not DaemonCapabilityMissingError, and it has no test. requireCapability calls this.capabilities(), a live network fetch: when /capabilities answers 500 (or the fetch rejects), deleting the instanceof rethrow would swallow the failure and silently strip sourceType/sourceId — a resume that must fail would proceed with a source-less body, and the two new tests (capability-present forward, capability-missing omit) cannot discriminate the behaviours. Add a DaemonSessionClient.test.ts case where /capabilities returns 500 and resume is called with source metadata; assert the resume rejects and no /session/:id/resume call is made — then remove the rethrow line and confirm the test goes red (a scratch-tree probe ran exactly that mutation: threw=false, reachedResume=true with the existing tests still green).
中文说明
R2-2：（由修复引入）针对 R2-2 的修复把强 requireCapability 门替换为这个 try/catch 优雅降级，并补充了省略测试，关闭了原报告的问题——但该改动引入了这个针对非 DaemonCapabilityMissingError 错误的重新抛出分支，而它没有测试。requireCapability 会调用 this.capabilities()，这是一次实时网络请求：当 /capabilities 返回 500（或 fetch 失败）时，删除这个 instanceof 重新抛出会吞掉失败并静默剥离 sourceType/sourceId——本应失败的 resume 会以不带来源的请求体继续执行，而两个新测试（有 capability 时转发、缺 capability 时省略）无法区分这两种行为。建议在 DaemonSessionClient.test.ts 中增加一个用例：让 /capabilities 返回 500，并以带 source 元数据的参数调用 resume；断言 resume reject 且没有发出 /session/:id/resume 调用——然后删除该重新抛出行，确认测试变红（临时树探针做过完全相同的变异：threw=false, reachedResume=true，而现有测试保持绿色）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Refresh the branch on the current main while keeping the restore source metadata safeguards and regression coverage intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Keep channel restore attribution complete when the bridge creates daemon sessions so source ids are not sent without their source type. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): Tests 87 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 1110 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 71 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 805 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 89 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; and 1 more.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.ts:6189 — [review] restore 回填缺少 parseSessionSource 校验（防御纵深）packages/acp-bridge/src/bridge.ts:7077 — [probe] coalesced-waiter 与 raced-entry 回填调用点无测试packages/cli/src/serve/routes/session.ts:3622 — [review] stripped-standalone 合并分支无测试packages/sdk-typescript/src/daemon/DaemonClient.ts:3409 — [probe] load 动作的 restore-source 正向转发零测试
Convergence: round 4 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/acp-bridge/src/bridge.ts (findings in round 3; 5 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：Tests 87 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 1110 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 71 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 805 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; Tests 89 passed — this review observed 1797, 1142, 25555, 1697, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 605, 4333, 639 passed; and 1 more。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 6 条行内评论，其中 6 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/bridge.ts（第 3 轮已出过发现，本轮又有 5 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| throw error; | ||
| } | ||
| } | ||
| const sourcePersisted = await applyRestoreSourceIfMissing(existing, req); |
There was a problem hiding this comment.
[Critical] R4-1: The new await applyRestoreSourceIfMissing(existing, req) sits after the final assertAttachableSessionEntry in restoreSession and returns attached: true plus a freshly registered clientId without re-checking liveness — the only await in this function with no post-await re-assert (the approval-mode block just above re-asserts and rolls back after its await). If the agent channel dies during the sessionSource ext-method round-trip — a window of up to initTimeoutMs (10s) — persistSessionSource swallows the transport-closed rejection and returns false while the channel.exited handler reaps the entry from byId; the caller is left bound to a session that 404s on every subsequent request. The same defect class exists at three sibling sites: the coalesced waiter (~7077), the raced-entry path (~7552), and the cold-restore persistSessionSource (~7690). Witness — probe driving the real bridge at HEAD, channel crashed mid-sessionSource:
BASE (PR code): {"resolved":"success","attached":true,"clientId":"client_ad1cc100-…","sourcePersisted":false,"summaryAfterCrash":"GONE-FROM-CATALOG (SessionNotFoundError)"}
WITH SUGGESTED FIX: {"resolved":"threw","error":"SessionNotFoundError: … The session channel is unavailable; retry after teardown completes"}
Re-run assertAttachableSessionEntry(...) after this await and before building the response, mirroring the approval-mode branch — including rollbackAttachRegistration on this existing-entry path. Fix acceptance: a bridge.test.ts case whose sessionSource ext method hangs and whose channel closes mid-call, asserting resumeSession rejects instead of resolving attached: true — removing the re-assert must turn it red.
中文说明
新增的 await applyRestoreSourceIfMissing(existing, req) 位于 restoreSession 中最后一次 assertAttachableSessionEntry 之后，却在返回 attached: true 和新注册的 clientId 前不再复核会话存活——这是该函数中唯一没有 post-await 复核的 await（上方 approval-mode 分支在其 await 后有复核 + 回滚）。若 agent channel 在 sessionSource ext-method 往返期间断开（窗口最长 initTimeoutMs = 10 秒），persistSessionSource 会吞掉 transport-closed 拒绝并返回 false，而 channel.exited handler 已把该 entry 从 byId 移除；调用方将绑定到一个后续每个请求都 404 的会话。同一缺陷类还有三个兄弟位置：coalesced waiter（~7077）、raced-entry 路径（~7552）、cold restore 的 persistSessionSource（~7690）。证据——在 HEAD 上对真实 bridge 的探针，sessionSource 往返中途令 channel 崩溃：见上方英文部分的探针输出（修复前解析为 attached: true 且目录中已查无该会话；应用建议修复后翻转为抛出 SessionNotFoundError）。修复建议：在该 await 之后、构造响应之前重新运行 assertAttachableSessionEntry(...)，参照 approval-mode 分支（此 existing-entry 路径同时回滚 attach 注册 rollbackAttachRegistration）。修复验收：新增 bridge.test.ts 用例——sessionSource ext method 挂起时关闭 channel，断言 resumeSession 拒绝而不是解析 attached: true；移除 re-assert 时该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| throw error; | ||
| } | ||
| } | ||
| const sourcePersisted = await applyRestoreSourceIfMissing(entry, req); |
There was a problem hiding this comment.
[Critical] R4-1 (location 2 of 4): Same missing post-await liveness re-assert on the coalesced-waiter settlement path — this await applyRestoreSourceIfMissing(entry, req) runs after the last assertAttachableSessionEntry and the waiter response is built without re-checking that the session is still attachable. If the channel dies during the sessionSource round-trip (window up to initTimeoutMs = 10s), the waiter receives a success response with a freshly registered clientId for a session the exited handler has reaped — every subsequent request on it 404s. Sibling sites: the existing-entry attach (~6917), the raced-entry path (~7552), and the cold-restore persist (~7690); the probe evidence in the ~6917 thread applies to this shape. Add assertAttachableSessionEntry(...) after the await (with the same rollback as the approval-mode branch), and pin it with a test that closes the channel mid-sessionSource and asserts rejection.
中文说明
R4-1（4 处之 2）：coalesced-waiter 结算路径存在同样的 post-await 存活复核缺失——此处的 await applyRestoreSourceIfMissing(entry, req) 位于最后一次 assertAttachableSessionEntry 之后，构造 waiter 响应前未再复核会话是否仍可 attach。若 channel 在 sessionSource 往返期间断开（窗口最长 initTimeoutMs = 10 秒），waiter 会收到一个针对已被 exited handler 移除会话的成功响应和新注册的 clientId——后续每个请求都 404。兄弟位置：existing-entry attach（~6917）、raced-entry 路径（~7552）、cold restore 持久化（~7690）；~6917 线程中的探针证据同样适用于此形状。修复：在该 await 之后加 assertAttachableSessionEntry(...)（与 approval-mode 分支相同的回滚），并用“sessionSource 中途关闭 channel 断言拒绝”的测试钉住。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const sourcePersisted = await applyRestoreSourceIfMissing( | ||
| racedEntry, |
There was a problem hiding this comment.
[Critical] R4-1 (location 3 of 4): Same missing post-await liveness re-assert on the raced-entry path — after this applyRestoreSourceIfMissing(racedEntry, req) await the response is returned without re-checking that the raced entry's channel is still alive. A channel death during the sessionSource round-trip (window up to initTimeoutMs = 10s) leaves the caller with attached: true and a freshly registered clientId bound to a reaped session that 404s on every subsequent request. Sibling sites: ~6917, ~7077, ~7690 (probe evidence in the ~6917 thread). Add assertAttachableSessionEntry(...) after the await, mirroring the approval-mode branch, and pin it with a mid-sessionSource channel-close test asserting rejection.
中文说明
R4-1（4 处之 3）：raced-entry 路径同样缺少 post-await 存活复核——此 applyRestoreSourceIfMissing(racedEntry, req) await 之后直接返回响应，未再复核 raced entry 的 channel 是否仍存活。若 channel 在 sessionSource 往返期间断开（窗口最长 initTimeoutMs = 10 秒），调用方会拿到 attached: true 和绑定到已被移除会话的新 clientId，后续每个请求都 404。兄弟位置：~6917、~7077、~7690（探针证据见 ~6917 线程）。修复：在 await 之后加 assertAttachableSessionEntry(...)（参照 approval-mode 分支），并用 sessionSource 中途关闭 channel 的测试断言拒绝。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const sourcePersisted = entry.sourceType | ||
| ? await persistSessionSource( |
There was a problem hiding this comment.
[Critical] R4-1 (location 4 of 4): The cold-restore persistSessionSource await is the fourth instance of the same class — it sits after the last assertAttachableSessionEntry(req.sessionId, entry) + registerClient, and the restore response (sessionId, fresh clientId, sourcePersisted) is built without re-asserting liveness. A fix scoped to the three applyRestoreSourceIfMissing call sites (~6917/~7077/~7552) leaves this one unguarded. Witness — probe driving a cold restore at HEAD with entry.sourceType set, channel crashed mid-sessionSource:
PR (unmodified): outcome.kind = "resolved" value = {"sessionId":"f8-cold-restore","attached":false,"clientId":"client_f2471801-…","sourcePersisted":false,…} summaryAfter = <threw SessionNotFoundError> (entry reaped, restore still succeeded)
with one-line fix (assertAttachableSessionEntry after the await): outcome.kind = "rejected", "SessionNotFoundError: … session channel is unavailable"
Add assertAttachableSessionEntry(req.sessionId, entry); after the await and before building the response. Fix acceptance: a cold-restore test that closes the channel while the sessionSource ext-method is in flight and asserts the resume rejects; removing the re-assert must turn it red.
中文说明
R4-1（4 处之 4）：cold-restore 的 persistSessionSource await 是同一缺陷类的第四处——它位于最后一次 assertAttachableSessionEntry(req.sessionId, entry) + registerClient 之后，构造恢复响应（sessionId、新 clientId、sourcePersisted）前未再复核存活。只修三处 applyRestoreSourceIfMissing 调用点（~6917/~7077/~7552）会漏掉这里。证据——在 HEAD 上对带 entry.sourceType 的 cold restore 打桩，sessionSource 中途崩溃 channel：见上方英文部分探针输出（未修复时解析为成功响应但 entry 已被 reap、summary 抛 SessionNotFoundError；加一行 re-assert 后翻转为拒绝）。修复：在该 await 之后、构造响应之前加 assertAttachableSessionEntry(req.sessionId, entry);。修复验收：新增 cold-restore 测试——sessionSource ext-method 挂起时关闭 channel，断言 resume 拒绝；移除 re-assert 时必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const sourcePersisted = entry.sourceType | ||
| ? await persistSessionSource( |
There was a problem hiding this comment.
[Critical] R4-2: The restore backfill persists the session_source record at the transcript's EOF — persistSessionSource → sessionSource ext-method → recordSessionSource/appendRecordStrict is append-only — but every daemon discovery path reads only the head window: readCreationMetadata scans MAX_PROMPT_SCAN_LINES = 10 records (sessionService.ts), and getSessionListItem/listSessions/session-list's persisted side use the same head scan, while the bridge catalog is in-memory and recoveryMode: 'lazy' restores nothing at startup. So a legacy session with ≥10 transcript records — essentially every real conversation — is backfilled, shows Channels while live, and silently falls back under Tasks after every daemon restart: the misclassification this PR fixes recurs. The route's hasPersistedSource no-overwrite guard (session.ts:3622) reads through the same head-scan and is blind for exactly these sessions, so caller source is forwarded again on later restores; the agent-side first-writer-wins guard (hydrated from the full transcript) keeps the persisted record correct, so the residual cost is transient live misattribution plus a mock-blind test — but the classification fix itself does not survive a restart. Witness — real SessionService probe at HEAD:
TAIL-source-after-12-records: readCreationMetadata={} listItem.sourceType=undefined
HEAD-source-at-record-2: readCreationMetadata={"sourceType":"channel","sourceId":"dingtalk-main"} listItem.sourceType="channel"
boundary sweep: [{9,visible:true},{10,visible:true},{11,visible:false},{12,visible:false}]
Make restore-time attribution discoverable where the product reads it — e.g. a tail-aware session_source read (scan from EOF before falling back to the head window) shared by readCreationMetadata/listing extraction and the route's hasPersistedSource guard, or persist backfilled attribution in a daemon-side store the merged listing consults. Fix acceptance: a sessionService test writing >10 records with the session_source record at the end, asserting readCreationMetadata()/listSessions() return the source — red today.
中文说明
R4-2：restore 回填把 session_source 记录持久化在 transcript 的末尾（persistSessionSource → sessionSource ext-method → recordSessionSource/appendRecordStrict 为仅追加），但 daemon 的所有发现路径只读头部窗口：readCreationMetadata 只扫描前 MAX_PROMPT_SCAN_LINES = 10 条记录（sessionService.ts），getSessionListItem/listSessions/session-list 持久化侧同样如此；而 bridge 目录是纯内存的，recoveryMode: 'lazy' 启动时不恢复任何会话。因此任何 ≥10 条记录的遗留会话（几乎所有真实会话）在回填后活体时显示 Channels，daemon 每次重启后又静默落回 Tasks——本 PR 要修复的错分类会反复复发。路由侧的 hasPersistedSource 防覆盖守卫（session.ts:3622）经由同一个 head-scan 读取，对这类会话同样失明，后续 restore 会再次转发调用方 source；好在 agent 侧 first-writer-wins 守卫从完整 transcript 补水合，持久化记录本身不会被改写，残余代价是临时性活体错归属和一个被 mock 蒙蔽的测试——但分类修复本身无法在重启后存活。证据——在 HEAD 上对真实 SessionService 的探针：见上方英文部分（记录在 12 条之后时 readCreationMetadata={}、列表项无 sourceType；记录在第 2 条时可见；边界扫描显示可见性恰在第 10/11 条之间翻转）。修复建议：让 restore 归属在产品读取它的地方可发现——例如尾部感知的 session_source 读取（先扫 EOF 再回退头部窗口），由 readCreationMetadata/列表抽取与路由 hasPersistedSource 守卫共用；或把回填归属写入 daemon 侧合并列表会查询的存储。修复验收：sessionService 测试——写 >10 条记录、session_source 在末尾，断言 readCreationMetadata()/listSessions() 返回 source（当前为红）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| * Source class stamped as daemon session metadata for new sessions and | ||
| * restore-time attribution for legacy sessions resumed through a channel. | ||
| */ | ||
| sourceType?: string; |
There was a problem hiding this comment.
[Suggestion] R4-3: This new sourceType option is dead plumbing: no production caller sets it — SessionRouter.sessionOptions() (the sole production options builder) sets only sourceId; only DaemonChannelBridge.test.ts passes it. DaemonChannelBridge.newSession/loadSession spread it into the factory request even though DaemonChannelSessionFactoryRequest declares no sourceType member (the spread bypasses TypeScript's excess-property check, so the mismatch is invisible to the type checker), and the only production factory, createDaemonSessionFactory (daemon-worker.ts), hardcodes sourceType: 'channel' on both branches and never reads req.sourceType. A plugin/adapter consumer answering this JSDoc's invitation — "Source class stamped as daemon session metadata" — gets its value silently discarded and overwritten with 'channel' while every test stays green; the two forwarding spreads are dead production code kept alive only by tests. Either delete the option and both spreads (the worker factory's hardcode is what actually stamps attribution), or declare sourceType on DaemonChannelSessionFactoryRequest and have the factory honor req.sourceType ?? 'channel', pinned by a daemon-worker test.
中文说明
R4-3：这个新增的 sourceType 选项是死管道：没有任何生产调用方设置它——SessionRouter.sessionOptions()（唯一的生产选项构造器）只设置 sourceId，只有 DaemonChannelBridge.test.ts 传入。DaemonChannelBridge.newSession/loadSession 把它 spread 进工厂请求，但 DaemonChannelSessionFactoryRequest 并未声明 sourceType 成员（spread 绕过 TypeScript 的多余属性检查，类型检查器看不到这一不匹配），而唯一的生产工厂 createDaemonSessionFactory（daemon-worker.ts）在两个分支都硬编码 sourceType: 'channel'、从不读取 req.sourceType。插件/适配器消费者若按此 JSDoc（“作为 daemon 会话元数据打标的来源类别”）的邀请传入该值，会被静默丢弃并覆盖为 'channel'，且所有测试保持绿色；两条转发 spread 是仅靠测试存活的死生产代码。二选一：删除该选项与两处 spread（worker 工厂的硬编码才是实际打标处）；或在 DaemonChannelSessionFactoryRequest 声明 sourceType 并让工厂尊重 req.sourceType ?? 'channel'，并用 daemon-worker 测试钉住。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Reject restores that lose their channel while source metadata is persisted, and make appended source records discoverable after restart. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-6 dead
sourceTypeplumbing at ChannelAgentBridge.ts:107 — still stands (file unchanged since round 4), already reported (comment 3879107104)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1a": run the new vitest suites ( packages/core/src/services/sessionService.test.ts , packages/acp-bridge/src/bridge.test.ts ) at the reviewed commit — the worktree ….
Test Plan (not a blocker): Tests 87 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 1110 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 71 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 805 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 89 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; and 1 more.
11 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 5, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.ts:7094 — [review] R5-2 (location 1 of 2): The new post-await liveness re-assert + rollback block on the coalesced-waiter path has no test coverage — the raced-entry path (~7580) is location 2. All 52 sourceT…packages/acp-bridge/src/bridge.ts:7583 — [review] R5-2 (location 2 of 2): Same missing test coverage on the raced-entry path — the coalesced-waiter site (~7092) is location 1. No existing test reaches this block with req.sourceType set (s…packages/core/src/services/sessionService.ts:1815 — [review] R5-3: This strict JSON.parse loses the two documented corruption shapes that the sibling tail reader readLastRecordUuid just below deliberately recovers via jsonl.parseLineTo…packages/acp-bridge/src/bridge.ts:6920 — [review] R5-4 (location 1 of 4): The post-await liveness re-assert + conditional approval-mode rollback + rollbackAttachRegistration + rethrow recipe is pasted into all four restore sites this diff…packages/acp-bridge/src/bridge.ts:7094 — [review] R5-4 (location 2 of 4): Same copy-pasted re-assert + rollback recipe as the existing-entry site (~6918) — this coalesced-waiter copy differs only in the sessionId argument ( restored.session…packages/acp-bridge/src/bridge.ts:7583 — [review] R5-4 (location 3 of 4): Same copy-pasted re-assert + rollback recipe — this raced-entry copy is the one that must pass the site-specific 1 + coalesceState.count delta to rollbackAttachReg…packages/acp-bridge/src/bridge.ts:7750 — [review] R5-4 (location 4 of 4): Same copy-pasted re-assert + rollback recipe on the cold-restore site (see location 1 for the proposed shared helper). Note this is the fourth place a fix must touch …packages/core/src/services/sessionService.ts:1784 — [review] R5-5: This hand-rolls a third copy of the 64-KiB tail-window scan (window math, partial-first-segment byte peek, scratch-buffer reuse, bottom-up walk) when readLastJsonStringFiel…packages/core/src/services/sessionService.ts:1836 — [review] R5-7: This catch-all returns {} on every I/O error (EACCES, EMFILE, EIO, …) with no log trail, while the head-read half of the same operation warns ( readCreationMetadataInterna…packages/core/src/services/sessionService.ts:1776 — [review] R5-8: The feature's committed design doc ( docs/design/2026-07-15-daemon-session-source-metadata.md , untouched by this PR) now describes behaviour the code no longer has: it says…packages/core/src/services/sessionService.test.ts:7027 — [review] R5-9: This new test never pushes the fixture past TAIL_READ_SIZE (64KB), so the tail reader's large-file branch — readStart > 0 , the peek byte check at readStart - 1 ,…packages/acp-bridge/src/bridge.ts:7740 — [probe] branchSession double-persist flips sourcePersistedpackages/acp-bridge/src/bridge.ts:6174 — [probe] [object Object] in persist-failure diagnostic
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1a"：run the new vitest suites ( packages/core/src/services/sessionService.test.ts , packages/acp-bridge/src/bridge.test.ts ) at the reviewed commit — the worktree …。
Test Plan（非阻断）：Tests 87 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 1110 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 71 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 805 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; Tests 89 passed — this review observed 1799, 22092, 25556, 1697, 605, 4333, 639 passed; and 1 more。
11 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 5 轮，非阻断）——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录，本轮不要求修改：共 13 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| try { | ||
| const fileSize = fs.statSync(filePath).size; | ||
| if (fileSize === 0) return {}; | ||
| const readStart = Math.max(0, fileSize - TAIL_READ_SIZE); |
There was a problem hiding this comment.
[Critical] R5-1: The tail-window fallback added this round covers only the last TAIL_READ_SIZE (64KB) of the transcript, and nothing maintains the head-or-tail invariant it relies on, so the #10248 misclassification silently returns for long-lived sessions. readCreationMetadata/listSessions/getSessionListItem now consult two windows: the first MAX_PROMPT_SCAN_LINES = 10 records and the last 64KB. A restore backfill appends the session_source record at EOF, and recordSessionSource is write-once — first-writer-wins, with currentSourceType re-hydrated from the full transcript on load — so it never re-appends. Once the conversation appends more than 64KB after the record, the record sits in neither window. custom_title solves the same drift by re-anchoring to EOF every 32KB (TITLE_REANCHOR_BYTES); session_source has no re-anchor, and sessionStorageUtils.ts documents that the writer must maintain this invariant. After any daemon restart the session lists under Tasks again and the restore-time lookup (readCreationMetadataIfReadable → live-session-source) can no longer recover the attribution — no later restore re-backfills, because the router no longer sends source for a session it cannot discover, so the loss is permanent. Witness — probe driving the real SessionService in a scratch tree: with head line + 11 fillers + the session_source record + ~90KB of further records, getSessionListItem/readCreationMetadata returned no sourceType/sourceId (the control arm with the record inside the window passed); widening only TAIL_READ_SIZE to 1MB made the same file return the source, proving only the 64KB boundary excludes it. Maintain the invariant at the writer — e.g. generalize updateTitleAnchorTracking/reanchorTitle so a persisted session_source is re-anchored to EOF alongside custom_title, or persist backfilled attribution in a daemon-side store the merged listing consults; a reader-side window alone, however large, is re-defeated by ordinary growth. Fix acceptance: extend the new rehydration test to append >64KB of filler AFTER the sessionSourceLine and still expect the source from listSessions()/readCreationMetadata() — it is red today, must pass after the fix, and must go red again if the fix is removed.
中文说明
本轮新增的尾部窗口回退只覆盖 transcript 最后 TAIL_READ_SIZE（64KB），且没有任何机制维护它所依赖的“头部或尾部”不变量，因此对长寿命会话来说 #10248 的错分类会静默复发。readCreationMetadata/listSessions/getSessionListItem 现在只查两个窗口：前 MAX_PROMPT_SCAN_LINES = 10 条记录和最后 64KB。restore 回填把 session_source 记录追加在文件末尾，而 recordSessionSource 是一次写入、先到先得（加载时 currentSourceType 从完整 transcript 补水合），不会再次追加；一旦会话在该记录之后又追加了超过 64KB 的内容，该记录就同时落在两个窗口之外。custom_title 用每 32KB（TITLE_REANCHOR_BYTES）重新锚定到 EOF 的方式解决了同样的漂移问题；session_source 没有对应的重锚定，而 sessionStorageUtils.ts 明确写明该不变量应由写入方维护。任何一次 daemon 重启后，该会话又会落到 Tasks 下，且 restore 侧的发现路径（readCreationMetadataIfReadable → live-session-source）再也找不回归属——后续 restore 也不会再回填（路由器对发现不到来源的会话不会再发送 source），因此丢失是永久性的。证据——在 scratch tree 中驱动真实 SessionService 的探针：头部行 + 11 条填充 + session_source 记录 + 约 90KB 后续记录的文件，getSessionListItem/readCreationMetadata 返回的条目没有 sourceType/sourceId（记录在窗口内的对照组通过）；仅把 TAIL_READ_SIZE 放宽到 1MB，同一文件即可返回来源，证明只是 64KB 边界把它排除。修复请把不变量放到写入侧——例如泛化 updateTitleAnchorTracking/reanchorTitle，让已持久化的 session_source 随 custom_title 一起定期重锚到 EOF；或把回填的归属写入 daemon 侧合并列表会查询的存储。只靠读取侧窗口（无论多大）都会被正常增长再次击穿。修复验收：扩展新增的回水合测试——在 sessionSourceLine 之后再追加 >64KB 填充，仍要求 listSessions()/readCreationMetadata() 返回来源；当前为红，修复后转绿，移除修复后必须再次变红。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Re-anchor durable source attribution as transcripts grow and remove the unused channel source-type option. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): Tests 87 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 1110 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 71 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 805 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 89 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; and 1 more.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/services/chatRecordingService.test.ts:2895 — [probe] restoreSessionState priming path has zero test (mutant survives)packages/core/src/services/chatRecordingService.ts:1577 — [review] reanchorSessionSource/reanchorTitle twin-copy drift riskpackages/core/src/services/chatRecordingService.ts:1577 — [probe] updateActiveTail:false on source re-anchor unpinnedpackages/core/src/services/sessionService.ts:1819 — [probe] bare null line aborts the whole tail scanpackages/core/src/services/sessionService.ts:1799 — [review] tail reader lacks the siblings' O_NOFOLLOWpackages/core/src/services/sessionService.ts:1813 — [review] listing parses the full 64KB tail for source-less sessions
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：Tests 87 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 1110 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 71 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 805 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; Tests 89 passed — this review observed 1142, 22094, 1799, 404, 229, 271, 207, 59, 2, 302, 18, 135, 88, 25557, 1697, 605, 4333, 639 passed; and 1 more。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
2 participants
What this PR does
This PR makes daemon channel session restores carry the same
sourceType: 'channel'and optionalsourceIdmetadata that new channel sessions already receive, but only for legacy sessions that do not already have persisted source metadata.The source attribution now crosses the full restore path:
sourceIdsourceType/sourceIdfor both create and restoresession_source_metadatacapability gate and falls back cleanly for older daemonssessionSourcecontrol methodWhy it's needed
Fixes #10248. DingTalk channel conversations created before source attribution existed can be resumed by the channel router without any channel metadata. The web shell sidebar classifies sessions without
sourceType: 'channel'as Tasks, so those resumed channel conversations keep appearing under Tasks instead of Channels.Persisting only missing source metadata preserves existing attribution for sessions that already recorded a source, while still repairing legacy/unattributed channel sessions when they are restored through a channel worker.
Reviewer Test Plan
How to verify
Run the targeted CLI, serve-route, SDK, ACP bridge, and channel bridge/router tests below. The important regression coverage is that restore metadata now crosses the SDK wire boundary, reaches the daemon restore route, is persisted for legacy/unattributed sessions, does not overwrite existing source metadata, and degrades gracefully when connected to a daemon without the
session_source_metadatacapability.Evidence (Before & After)
Before: the restore path only passed source metadata to a mocked
DaemonSessionClient.resume()boundary. The real SDK restore transport serialized onlycwd/approvalMode/ load-only fields, the daemon route did not parse restore-time source metadata, and legacy channel sessions could remain unclassified after daemon restart.After:
DaemonSessionClient.loadSession()/resumeSession()accept source metadata, the SDK sends it inPOST /session/:id/{load,resume}when the daemon advertises support,restoreSessionHandlervalidates and applies it only when the persisted metadata is missing, and ACP bridge persists that backfill throughsessionSourceso the sidebar can classify restored channel sessions after restart.Local verification on current head
1fefb5c30f2d772c5d70429c0c6ca94d194c332e:Tested on
Environment (optional)
Node.js local workspace on macOS. The change spans channel restore plumbing, SDK restore serialization, daemon restore parsing, and ACP bridge source persistence.
Risk & Scope
session_source_metadatacapability continue restoring sessions without source metadata instead of rejecting the request.Linked Issues
Fixes #10248
中文说明
What this PR does
这个 PR 让 daemon channel session 的恢复路径携带和新建路径相同的
sourceType: 'channel'以及可选sourceId元数据，但只会给尚未持久化 source metadata 的 legacy session 回填，不覆盖已经记录过来源的 session。现在来源归属已经贯穿完整恢复链路：
sourceId透传sourceType/sourceIdsession_source_metadatacapability gate 后序列化这两个字段，并对旧 daemon 干净降级sessionSourcecontrol method 持久化Why it's needed
修复 #10248。来源归属能力出现之前创建的钉钉 channel 会话，可能被 channel router 恢复时仍然没有 channel metadata。web shell 侧边栏会把没有
sourceType: 'channel'的会话归到 Tasks，因此这些恢复后的 channel 会话会继续出现在 Tasks，而不是 Channels。只在缺失来源时持久化回填，既能修复 legacy/unattributed channel sessions，又不会改写已经记录过来源的 session。
Reviewer Test Plan
How to verify
运行下面的 CLI、serve route、SDK、ACP bridge、channel bridge/router 针对性测试。关键回归覆盖点是：restore metadata 现在会跨过 SDK wire boundary，到达 daemon restore route，能为 legacy/unattributed sessions 持久化回填，不覆盖已有 source metadata，并且连接不支持
session_source_metadatacapability 的旧 daemon 时会干净降级。Evidence (Before & After)
Before：restore 路径只把 source metadata 传到了 mock 的
DaemonSessionClient.resume()边界。真实 SDK restore transport 只序列化cwd/approvalMode/ load-only 字段，daemon route 也不会解析 restore-time source metadata，legacy channel sessions 在 daemon 重启后仍可能无法分类。After：
DaemonSessionClient.loadSession()/resumeSession()接收 source metadata，SDK 在 daemon 声明支持时通过POST /session/:id/{load,resume}发送它；restoreSessionHandler会校验并且只在持久化 metadata 缺失时应用它；ACP bridge 通过sessionSource持久化这个回填，所以 sidebar 能在重启后继续把恢复的 channel sessions 分到 Channels。当前 head
1fefb5c30f2d772c5d70429c0c6ca94d194c332e本地验证：Tested on
Environment (optional)
macOS 本地 Node.js 工作区。改动覆盖 channel restore plumbing、SDK restore serialization、daemon restore parsing 和 ACP bridge source persistence。
Risk & Scope
session_source_metadatacapability 的旧 daemon 会继续恢复 session，只是不发送 source metadata，不会因此拒绝请求。Linked Issues
Fixes #10248