refactor(cli): Generalize the Conversations runtime foundation - #8890
Conversation
|
Thanks for the PR — re-run triage at head Template looks good ✓ Problem: this is PR0 of the standalone daemon sessions design (issue #8908, labeled Direction: aligned. Future projectless sessions need this runtime without a second runtime or an ACP child per chat, and this PR establishes exactly that seam while staying behaviorally invisible — no routes, no capability, no new source type; advertisement is deferred to PR3 by the design. CHANGELOG: no direct reference, but the area is on the roadmap via #8908. Size: not a core-module change (no Approach: matches my independent proposal — move the workspace/source helpers out of the Live namespace, one one-flight Risk: Stage 1e high-risk path matched — Moving on to code review. 🔍 中文说明感谢贡献——这是在 head 模板完整 ✓ 问题：这是 standalone daemon sessions 设计的 PR0（issue #8908，带 方向：对齐。未来无项目会话需要复用该 runtime，且不能引入第二套 runtime 或每会话一个 ACP 子进程；本 PR 恰好建立这条接缝，同时保持行为不可见——无路由、无 capability、无新 source type，按设计推迟到 PR3 才广播。CHANGELOG：无直接引用，但该方向通过 #8908 在路线图上。 规模：非核心模块变更（未触及 方案：与我独立提出的方案一致——把 workspace/source helper 迁出 Live 命名空间，一个带 fail-closed 重校验与采用逻辑的 one-flight 风险：Stage 1e 高风险路径命中—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
📄 中文说明📄 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
4de39f3 to
6d036ae
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
doudouOUC
changed the title
🩺 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 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.10)
Clarify standalone transaction recovery outcomes and remove the unused Conversations-only Live Host workflow trigger. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
[codex] Addressed all 9 actionable R1 review suggestions in
Verification on the pushed commit:
The PR remains Ready for review; no implementation scope was added beyond the review fixes. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— gpt-5.6-sol via Qwen Code /review (v0.21.10)
Harden owned runtime publication and complete the standalone transaction safety contract. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving PR0 as a behavior-preserving generalization. Verified against head d50dbea.
Refactor correctness (checked line-by-line vs. the old inline logic in server.ts)
- Cached / existing-entry / publication paths all preserve the old gate semantics (provenance
live-conversation,trusted,removable === false, dev/ino root identity, entry state + generation identity). One intentional difference: a non-active entry at the Conversations root now rejects with "no longer an active owned runtime" instead of falling through topublishOwnedRuntime, which rejected with "Workspace is already registered" — both reject, no behavior regression. - The added post-publication re-verification in
ConversationRuntimeManageris strictly more defensive than the old code (which never re-checked afterpublishOwnedRuntime); on failure it fails closed. - Handler binding now rolls back cleanly on partial bind, and teardown clears all three bridge handlers via
liveBoundRuntime; ordering is safe (workspaceManagementHandleis unconditionally initialized at server.ts:2109 before anyensure()can fire — boot publication is at :2130, route handlers post-setup). - Renames are pure:
session-source.*100% moves;conversation-workspace.tsis identifier renames only (logic diffed, unchanged).invalidateServeFeaturesCachemoving before handler bind only changes transient cache state; the partial-bind test pins the new behavior.
On the open Criticals in the prior automated review — I verified both code findings independently:
assertOwnedRuntimenot rejectingruntime.primary: not reachable in this PR. The runtime factory setsprimary: buildOptions?.primary ?? falseandpublishOwnedRuntimenever passesprimary; a pre-existing primary entry at the same cwd fails the provenance gate. The gate is also unchanged from the pre-refactor code. Reasonable defense-in-depth for PR1, not a blocker here.- Post-publication validation leaving a registered runtime: only triggerable by a mid-call root-identity swap (inFlight + registry re-checks already guard concurrency), and the outcome is fail-closed — no crash, no misrouting. The practical case (bind failure after publication) is deliberate reuse-on-retry and is asserted by the new
rolls back a partial Live bindtest. Worth a cleanup path eventually; P2 at most. - The remaining findings target
docs/design/standalone-daemon-sessions.mdcontracts (UUID admission, journal identity binding/discovery, per-session pinning, rollback re-read) that belong to PR1–PR3 scope and are not implemented by this PR. Good follow-up questions for issue #8908 / the later PRs, not blockers for the foundation refactor.
Nits (P3): consider adding runtime.primary to the ownership gate as cheap insurance when PR1 lands; consider a disposal path for a runtime whose post-publication re-verification failed.
Tests: manager suite covers one-flight publication, adoption, cached-removal/replacement rejection, and retry-after-failure; server.test.ts adds concurrent-bind sharing, partial-bind rollback + retry-without-republish, and the speakHandler teardown assertion. Coverage matches the refactor surface.
CI: all checks green on head sha (only the in-progress review-pr job, i.e. this review, is pending).
|
[codex] Addressed all 8 actionable R2 findings in
Verification completed on the pushed commit:
|
|
@qwen-code /triage |
|
Addressed all five R4 review suggestions in
Verification: 146 manager/workspace-publication tests; 18 focused Live/publication/404 regressions; the independently rerun extension 404 test; CLI typecheck; Prettier; |
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.
Not reviewed: build-and-test — the macOS/Windows unit test legs never produced a verdict at this head; the changed serve paths were verified on Linux only.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I intended completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — full chunk read and all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget., and 4 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — the macOS/Windows unit test legs never produced a verdict at this head; the changed serve paths were verified on Linux only。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I intended completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — full chunk read and all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.，另有 4 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed all three R5 deletion-recovery suggestions in
Verification: Prettier; |
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: reverse audit — did not converge within the reverse-audit round cap of 5.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — the macOS/Windows unit test legs never produced a verdict at this head; the changed serve paths were verified on Linux only.
Not explored to full depth (tool budget reached): "PR #8890 generalizes the Conversations runtime foundation:…": none — all checks above completed within budget.; "PR #8890 generalizes the Conversations runtime foundation…": none — all checks I identified were completed within budget (11 tool calls).; "PR #8890 generalizes the Conversations runtime foundation:…": none — all checks I started were completed within budget.; "PR #8890 generalizes the Conversations runtime foundation…": none — all planned checks completed within budget.; "PR #8890 generalizes the Conversations runtime foundation…": none — all checks I started completed within budget. By chunk assignment I did not read doc lines ~392–993 (other agents' chunks); the finding's uncertainty abo…, and 3 more.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — the macOS/Windows unit test legs never produced a verdict at this head; the changed serve paths were verified on Linux only。
未探索到全部深度（达到工具调用预算）："PR #8890 generalizes the Conversations runtime foundation:…"：none — all checks above completed within budget.；"PR #8890 generalizes the Conversations runtime foundation…"：none — all checks I identified were completed within budget (11 tool calls).；"PR #8890 generalizes the Conversations runtime foundation:…"：none — all checks I started were completed within budget.；"PR #8890 generalizes the Conversations runtime foundation…"：none — all planned checks completed within budget.；"PR #8890 generalizes the Conversations runtime foundation…"：none — all checks I started completed within budget. By chunk assignment I did not read doc lines ~392–993 (other agents' chunks); the finding's uncertainty abo…，另有 3 条。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed review round 6 in
Verification: Prettier; |
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 — Test (macos-latest, Node 22.x) was skipped in CI and the changed serve paths were not verified on macOS at this head.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the changed serve paths were not verified on Windows at this head.
Not explored to full depth (tool budget reached): chunk 3: did not read the exact startCronScheduler call site to prove it fires strictly *before* managed relocation in current code (verified only structurally via ste…; chunk 3: did not trace updateSessionMetadata to the transcript write to confirm "appends" is the literal persistence mechanism behind the rename sentence; no concrete …; "PR #8890 generalizes the Conversations runtime foundation:…": none — all planned checks completed. I did not execute the test suite (outside the tracer dimension); the verification above is static cross-file tracing agains…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed (~13 tool calls).; "You are review agent reverse-audit — Reverse audit agent…": none — finished within budget; no check left unfinished., and 3 more.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and the changed serve paths were not verified on macOS at this head。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and the changed serve paths were not verified on Windows at this head。
未探索到全部深度（达到工具调用预算）：chunk 3：did not read the exact startCronScheduler call site to prove it fires strictly *before* managed relocation in current code (verified only structurally via ste…；chunk 3：did not trace updateSessionMetadata to the transcript write to confirm "appends" is the literal persistence mechanism behind the rename sentence; no concrete …；"PR #8890 generalizes the Conversations runtime foundation:…"：none — all planned checks completed. I did not execute the test suite (outside the tracer dimension); the verification above is static cross-file tracing agains…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed (~13 tool calls).；"You are review agent reverse-audit — Reverse audit agent…"：none — finished within budget; no check left unfinished.，另有 3 条。
未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/live-host.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Local re-verification report (real daemon, head
|
| Finding | Was | Now |
|---|---|---|
F1 — live-host.yml didn't watch serve/conversations/** |
blocker | Fixed. git diff 675dd458..9d087621 -- .github/workflows/live-host.yml adds the packages/cli/src/serve/conversations/** path filter, so the PR-body claim is now true. |
F2 — cached-reuse assertExactRoot unpinned |
minor | Fixed, mutation-verified. Deleting the cached-path assertExactRoot call now fails the manager suite (24 tests, 1 failure; previously 18 tests all stayed green). |
F3 — must not be primary admission guard untested |
minor | Fixed, mutation-verified. Deleting the guard now fails rejects and disposes a primary owned-runtime candidate in workspace-management.test.ts. |
| F4 — transient visible-but-untrusted publication window | note | Fixed by construction. f00873c2fd renames the hook to validateBeforePublication and runs it before any registry insertion; pinned by the new keeps a candidate unpublished and the topology lock free during final validation test. |
| F5 — fail-closed boot publication is silent | note | Still open. server.ts still swallows the boot error with .catch(() => undefined); empirically my H4/H5 fail-closed arms wrote nothing to the daemon log. Fine to leave as a follow-up observability nit. |
2. Real-daemon matrix on the new head — 6 arms
| Arm | Setup | Observed |
|---|---|---|
| H1 | Live off (default settings) | Conversations root never created; GET /capabilities has only the primary workspace; 1 ACP child. |
| H2 | Live on | Root created 0700 owner-only; runtime advertised as primary:false, trusted:true, removable:false, kind:"live"; GET /workspace-registrations entries stay [] (daemon-owned runtime is not persisted); still 1 ACP child — boot publication registers runtime + bridge without preheating a second ACP process. |
| H3 | Live on + QWEN_SERVE_ACP_HTTP=0 |
Root never created, no live workspace, daemon healthy. |
| H4 | Pre-existing root chmod 0755 |
Fail closed: no runtime published, no replacement, no fallback to primary; root left untouched (same inode and mode after boot); daemon stays healthy. |
| H5 | Root replaced by a symlink | Fail closed: symlink left intact, refused; daemon healthy. |
| H6 | Restart daemon against the H2 root | Root adopted, not recreated (same inode 161922030), republished as kind:"live"; still 1 ACP child; the previous daemon exited cleanly on SIGTERM. |
3. Base/head parity and ACP laziness
BASE ran the same Live-off/Live-on arms. After normalizing /capabilities (drop qwenCodeVersion and workspace ids, mask $HOME), the JSON is byte-identical between base and head in both arms, and every arm on both binaries runs exactly 1 ACP child — enabling Live adds a workspace but never a process.
4. Restart adoption, shutdown, and test suites
On this head: the 6 focused files pass 176/176 (conversation-runtime-manager 24 — includes the concurrent boot/Host-bind one-flight test, workspace-management 122, conversation-workspace, session-source, live-task-service 14, live-worker-workspace 5), and server.test.ts -t 'Live' passes 20/20.
5. Web Shell — still no user-visible change
| HEAD, Live on | BASE, Live on | HEAD, Live off |
|---|---|---|
 |
 |
 |
Real Chrome against each daemon: with Live on, the sidebar shows the same Live section on both binaries; with Live off it disappears (and the voice button leaves the composer). No visual delta.
Not verified
Unchanged from last time: Windows/Linux, and the real Live Host native binding (no Host binary / DashScope Realtime credential here) — handler bind/teardown and the boot-vs-Host-hello one-flight remain covered by the unit suites only.
Verdict: the previously blocking F1 is fixed, F2–F4 are demonstrably closed, and the refactor is behaviour-preserving on every end-to-end observable I could measure on this head. LGTM as a merge reference; F5 can be a follow-up.
中文版
本地复验报告（真实 daemon，head 9d087621）
这是对我 2026-08-12 报告（当时测的是 ecfd2949）的跟进。此后 PR 又经过六轮 review，所以我把当前 head 从零重新构建，重跑了整套真实 daemon 验证矩阵，并针对性复查了之前的五个 findings。结论：F1–F4 在当前 head 上确认已修复（F2/F3 是重新做变异测试验证的，不是只看 diff）；F5 仍未处理但只是备注级。新一轮端到端矩阵——包括新增的 fail-closed 和重启采用臂——全绿，base/head 行为对等仍然成立。我这边认为可以合并。
环境。 macOS 25.6.0（arm64）、Node v24.18.1。两个隔离 worktree，各自全新 npm ci + npm run build + npm run bundle：BASE 675dd458（与 origin/main 的 merge-base）与 HEAD 9d087621。每个 daemon 跑在独立 $HOME 下，~/Documents/Qwen Code/Conversations 每臂完全沙箱化。
1. 上轮 findings 在当前 head 的复查
| Finding | 原级别 | 现状 |
|---|---|---|
F1 — live-host.yml 未监听 serve/conversations/** |
阻塞 | 已修复。 git diff 675dd458..9d087621 -- .github/workflows/live-host.yml 新增了 packages/cli/src/serve/conversations/** 路径过滤，PR 正文的说法现在成立。 |
F2 — 缓存复用路径的 assertExactRoot 无测试钉住 |
次要 | 已修复，变异验证。 删掉缓存路径的 assertExactRoot 调用后 manager 套件现在失败（24 例中 1 失败；此前 18 例全绿）。 |
F3 — must not be primary 准入守卫无测试 |
次要 | 已修复，变异验证。 删掉守卫后 workspace-management.test.ts 的 rejects and disposes a primary owned-runtime candidate 失败。 |
| F4 — 发布过程中短暂可见的 untrusted 窗口 | 备注 | 结构性修复。 f00873c2fd 把校验钩子改名为 validateBeforePublication 并移到注册之前执行；由新测试 keeps a candidate unpublished and the topology lock free during final validation 钉住。 |
| F5 — fail-closed 的 boot 发布完全静默 | 备注 | 仍未处理。 server.ts 仍用 .catch(() => undefined) 吞掉 boot 错误；实测 H4/H5 两个 fail-closed 臂在 daemon 日志里零输出。可作为后续可观测性小改进。 |
2. 新 head 上的真实 daemon 矩阵——6 个臂
| 臂 | 配置 | 观测结果 |
|---|---|---|
| H1 | Live 关（默认配置） | Conversations root 从未创建；GET /capabilities 只有 primary workspace；1 个 ACP 子进程。 |
| H2 | Live 开 | root 以 0700 仅属主权限创建；runtime 以 primary:false, trusted:true, removable:false, kind:"live" 发布；GET /workspace-registrations 的 entries 保持 []（daemon 自有 runtime 不持久化）；仍只有 1 个 ACP 子进程——boot 发布只注册 runtime 和 bridge，不预热第二个 ACP 进程。 |
| H3 | Live 开 + QWEN_SERVE_ACP_HTTP=0 |
root 从未创建，无 live workspace，daemon 健康。 |
| H4 | 预置 root chmod 0755 |
fail closed：不发布 runtime、不建替代、不回落 primary；root 原样保留（boot 前后 inode 和权限位相同）；daemon 保持健康。 |
| H5 | root 被替换为符号链接 | fail closed：符号链接原样保留、被拒绝；daemon 健康。 |
| H6 | 以 H2 的 root 重启 daemon | root 被采用而非重建（inode 161922030 不变），重新以 kind:"live" 发布；仍 1 个 ACP 子进程；前一个 daemon 收到 SIGTERM 后干净退出。 |
3. base/head 对等与 ACP 懒加载
BASE 跑了同样的 Live 关/开两臂。将 /capabilities 规范化（去掉 qwenCodeVersion 和 workspace id、掩码 $HOME）后，两臂的 JSON 在 base 与 head 之间逐字节一致；两个二进制的每个臂都恰好只有 1 个 ACP 子进程——开启 Live 增加的是 workspace，而不是进程。
4. 重启采用、关停与测试套件
当前 head 上：6 个聚焦测试文件 176/176 通过（conversation-runtime-manager 24 例——含 boot 与 Host 并发绑定共享同一次发布的 one-flight 用例，workspace-management 122 例，conversation-workspace、session-source、live-task-service 14 例、live-worker-workspace 5 例）；server.test.ts -t 'Live' 20/20 通过。
5. Web Shell——依旧无用户可见变化
真实 Chrome 分别连接各 daemon：Live 开启时两个二进制的侧栏显示相同的 Live 分区；Live 关闭时该分区消失（输入框的语音按钮也一并消失）。无视觉差异。
未验证范围
与上轮相同：Windows/Linux，以及真实 Live Host 原生绑定（本机没有 Host 二进制和 DashScope Realtime 凭据）——handler 绑定/清理与 boot vs Host hello 的 one-flight 仍仅由单测覆盖。
结论： 此前阻塞的 F1 已修复，F2–F4 已可证地关闭，且在我能测到的每个端到端可观测量上这次重构保持行为不变。作为合并参考：LGTM；F5 可以作为后续跟进。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2497 passed · 0 failed · 2497 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2497 通过 · 0 失败 · 2497 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2497 passed · 0 failed · 2497 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2497 通过 · 0 失败 · 2497 总计 Verification reportPR 8890 Deep Verification (follow-up round 3) —
|
| # | Finding (previous round) | Severity | Status at new head 9d087621 |
|---|---|---|---|
| F1 | New runtime.primary publication guard unpinned (M7 survivor) |
Suggestion | Fixed — the delta's test(cli): Address Conversations review coverage added rejects and disposes a primary owned-runtime candidate; M7 (guard deleted) now yields 1 failed | 121 passed, killed by exactly that test. |
| F2 | Server-level binding one-flight unpinned (M9 survivor) | Suggestion | Stands — re-measured: deleting if (liveBindingPromise) return liveBindingPromise; leaves 936/936 green. Classification unchanged (coverage gap, not a defect: without the dedup two concurrent callers bind the same idempotent setters twice). |
| F3 | Redundant state !== 'active' || !existing clause in adopt path |
Observation | Stands — M2/M2a survive (24/24 green), M2b (drop the follow-up assertActiveRuntime) killed by rejects an adopted runtime that stops being active during validation. Defense-in-depth, harmless. |
| C1 | Round-1 correction: description's workflow sentence stale | Correction | Superseded at round 2, still accurate here — re-verified at this head: the conversations/** paths entry is present and the filter change is monotonic (+1/-0 lines), so the sentence holds (W2–W4). |
| C2 | Round-2 correction: base fails draining/blocked at admission, not after a throwaway runtime | Correction | Re-confirmed — Arm 3 base cells C2/C3 reproduce Workspace is already registered with factoryCalls=0 and the entry untouched. |
Central claim and A/B proof
Central claim (unchanged): the PR moves the Conversations runtime lifecycle out of Live's closure into a reusable ConversationRuntimeManager while preserving Live behavior — publication is one-flighted under concurrency, adoption fails closed for anything but an active owned runtime, Live handler binding is transactional with rollback and retry-without-republication, ensure() stays lazy (no ACP preheat, no handler installation), and — the delta's contract — pre-publication validation keeps the candidate unpublished (dispose on failure, no trust lock held).
Delta claim (new since round 2, D1): fix(cli): Keep owned runtime validation unpublished + its tests pin validation-before-publication ordering; test(cli): Address Conversations review coverage pins the primary guard (F1) and the rollback paths. Per-commit attribution is out of reach (depth-2), so the delta is verified as an aggregate.
Arm 1 — behavior preservation (existing suites, both arms, re-measured)
| Suite set | Base (HEAD^1 = c0e649b53c) |
Head (merge ref) | Δ |
|---|---|---|---|
| conversation-workspace/session-source/workspace-management/live-task/live-worker (+manager on head) | 149/149 pass (5 files) | 176/176 pass (6 files) | +24 manager tests, +3 publication tests |
server.test.ts + multi-workspace-sessions.test.ts |
1034/1034 pass (933+101) | 1037/1037 pass (936+101) | +3 server tests (boot-pub share, 2× rollback it.each(['task','speech'])) |
Every test that exists on both arms passes on both arms; the deltas are exactly the PR's new tests. The new base (#9055, selective session restore) shifted the absolute counts (base 933 vs round-2's 932) but introduced no cross-arm failures. Witness: 01-ab-suites-head-vs-base.png. Raw logs: logs/base-focused.log, logs/base-server-mw.log, logs/head-focused.log, logs/head-server-mw.log.
Arm 2 — mechanism probes on the real compiled head code
manager-harness.mjs (57 assertions, all pass) drives the actual dist/ modules — ConversationRuntimeManager, createWorkspaceRegistry, the real publishOwnedRuntime handle obtained through registerWorkspaceManagementRoutes on a real express app, and a real ConversationWorkspace rooted in a temp home dir — with call-counted injected deps:
| Probe group | Result |
|---|---|
H1: 5 concurrent ensure() during a gated publication |
same in-flight promise; 1 publication; entry active; cache invalidated once |
| H2: cached reuse | same runtime; still 1 publication |
| H3: adopt active owned runtime | adopted by identity, 0 publications |
| H4/H5: draining / blocked entry | rejects no longer an active…, 0 publications, entry untouched |
| H6–H9: wrong provenance / untrusted / removable / primary at root | rejects already registered without Live provenance, 0 publications |
| H10/H11: cached runtime replaced / drain-completed | rejects fail-closed, 0 publications (entry removal invisible in registry) |
| H12/H13: factory failure / ownership-gate failure | retryable; invalid candidate never registered, disposed (factory-throw case disposes nothing — no object existed) |
| H13e: factory returns a primary candidate | rejected by the must not be primary guard before the validate callback runs, disposed, never registered |
| H14: real workspace over temp home | exact-root gate rejects a wrong-cwd candidate, disposed, never registered |
| H15: bridge instrumentation | preheat and all 3 Live handler setters called 0 times (lazy contract holds) |
| H16 (delta): validate gated in flight | no registry entry and no trust lock while validation pending; publish completes after release; trust lock taken once, after validation |
| H17 (delta): validation failure | candidate unpublished, disposed, runtimeAdded never called; retry succeeds (inFlight not stuck) |
Witness: 02-manager-harness-head.png.
Arm 3 — base-contrast, re-measured with real base modules
The base arm runs inside the base worktree via tsx, importing the real base registerWorkspaceManagementRoutes (with base publishOwnedRuntime and assertOwnedRuntimeAdmission), base createWorkspaceRegistry, and base LiveConversationWorkspace; the ensure-side algorithm is reconstructed verbatim from the code the PR deleted out of server.ts. The head arm drives the real head manager through identical scenarios. 20/20 assertions.
| Cell | Base algorithm (real base modules) | Head manager |
|---|---|---|
| C1 active control | adopts by identity, 0 publications | adopts by identity, 0 publications |
| C2 draining entry | rejects Workspace is already registered at admission, 0 runtimes created, entry untouched |
rejects no longer an active owned runtime at manager level, 0 runtimes created, entry untouched |
| C3 blocked entry | same admission rejection, 0 runtimes created | same manager-level rejection, 0 runtimes created |
| C4 foreign provenance | rejects already registered without Live provenance |
same message |
Both arms fail closed with zero runtimes created — consistent with round-2's Correction 2. Witness: 03-base-contrast-both-arms.png.
Mutation matrix (vacuity + guard attribution, re-run)
Unmutated controls in the mutation worktree: manager + workspace-management 146/146 green; server.test.ts 936/936 green. Positive control M3 (disable ownership predicate) turned 12 tests red — the harness can fail these suites. Every mutant was applied as a single exact-match edit in a scratch worktree and restored afterward; the three mutated files were verified byte-identical to HEAD at the end.
| Mutant | Guard removed | Suite result | Verdict | vs round 2 |
|---|---|---|---|---|
| M1 | manager one-flight dedup | 1 failed | 23 passed | KILLED | same |
| M2 | adopt state !== 'active' || !existing clause |
24 passed | SURVIVED | same (F3) |
| M2a | finer: keep only !existing |
24 passed | SURVIVED | same (F3) |
| M2b | finer: drop adopt-branch assertActiveRuntime |
1 failed | KILLED | same |
| M3 | ownership predicate (positive control) | 12 failed | 12 passed | KILLED | same (8 red on the smaller round-2 suite) |
| M4 | post-publication active assertion | 1 failed | KILLED | same |
| M5 | pending cleared on failure (retry) | 7 failed | 17 passed | KILLED | same |
| M7 | runtime.primary publication guard |
1 failed | 121 passed | KILLED | was SURVIVED (F1) — fixed by the delta's new test |
| M10 (new) | validation moved after registry.add (ordering) |
3 failed | 119 passed | KILLED | new this round — pins the delta fix |
| M8 | handler cleanup on partial Live bind | 2 failed | 934 passed | KILLED | same (the two rollback it.each tests) |
| M9 | server-level binding-promise dedup | 936 passed | SURVIVED | same (F2) |
11/11 mutants behaved as predicted. Witness: 04-mutation-matrix.png.
Delta verification — live-host.yml (D2, re-verified)
The filter still contains - 'packages/cli/src/serve/conversations/**'; the change vs base is exactly +1 line / −0 lines (monotonic superset — trigger coverage cannot regress). Strict YAML parse passes; trigger simulation over git diff --name-only HEAD^1..HEAD shows all 6 changed conversations/** files and all 4 changed live/** files match; the 10 non-matching changed files were never in this filter on base either. Witness: 05-workflow-gates.png.
Corrections
- Correction to the round-2 report's actionlint methodology claim (report error, not PR error). Round 2 stated actionlint ran "with shellcheck" and that a planted SC2086 was caught. The repo's
scripts/lint.jsinvokes actionlint with-shellcheck=(shellcheck integration disabled), so SC2086 can never fire through that gate — re-probing this round with an SC2086-style violation exited 0. The gate that is live is actionlint's native syntax/structure checking: a planted structural error (step with neitherrunnoruses) was caught (exit 1), and the repo is clean under it (exit 0). The round-2 conclusion "actionlint clean" therefore rests on actionlint-native checks only; thelive-host.ymldelta is apaths:entry with no shell content, so nothing material changes for this PR.
Findings
Carried forward, re-measured (no Critical or Major; none are merge conditions):
- Suggestion — server-level binding one-flight is unpinned (F2, stands). Deleting
if (liveBindingPromise) return liveBindingPromise;leaves 936/936 green (M9): the newshares the boot publication with a concurrent Live Host bindtest pins the manager's one-flight (M1 kills it), and without the server dedup two concurrent callers simply bind the same idempotent setters twice. A test asserting setter-call counts under concurrent boot+hello would pin it. - Observation — redundant clause in the manager's adopt path (F3, stands, harmless).
entry.state !== 'active' || !existingis fully subsumed by theassertActiveRuntimecall two lines later (M2/M2a survive, M2b killed). Defense-in-depth; no action needed.
No injection attempts detected in PR content.
Not covered
- Per-commit attribution. Depth-2 checkout:
git rev-list HEAD^1..HEAD^2yields 1 while the metadata snapshot lists 12 commits;.git/shallowconfirms the graft. The aggregateHEAD^1..HEADdiff was verified; which hardening landed in which of the five new commits was not separable. - yamllint could not be installed (no
pip3in the container;scripts/lint.js --setupfailed that one tool). Substitute: strict YAML parse + actionlint's own schema checks. - The Live Host job's payload itself (
bun install,live-host:typecheck/test/build, electron-builder packaging) runs onmacos-latestand was not executed in this Linux container; the delta touches only the trigger filter, not the job body. - End-to-end ACP preheat absence verified at unit/dist level only (H15 bridge instrumentation), not by booting a daemon against a real ACP child.
- Windows path behavior (
isSamePathcase-insensitivity) — Linux container only; matches the author's declared scope. - Design doc content (
docs/design/standalone-daemon-sessions.md, +1032 lines) — documentation, not behaviorally verified. - Full repo-wide test suite / repo-wide lint. Gates were scoped to the affected surface (19 changed files ESLint, cli
tsc --noEmit, actionlint repo-wide, 8 affected test files on both arms).
Methodology
Environment: the CI verify container (node:22-bookworm, Node v22.23.2), working tree at refs/pull/8890/merge with npm ci + npm run build pre-run; the snapshot's baseRefOid (33c330d0…) has drifted from the merge ref's base tip — per the merge-ref contract HEAD^1 (c0e649b53c) is authoritative and was used as the A/B base. Base arm: git worktree add tmp/base-tree HEAD^1 reusing the root and package-local node_modules via symlinks (the PR changes no lockfile/package.json entries; git diff HEAD^1..HEAD outside packages/cli + docs + .github is empty, so the shared workspace links the base side resolves are content-identical — realpath of node_modules/@qwen-code/qwen-code-core and @qwen-code/acp-bridge asserted as resolving into the head tree's packages/core / packages/acp-bridge, whose diff is empty); vitest's own aliases additionally resolve @qwen-code/qwen-code-core to each tree's local packages/core source. The base-contrast base arm ran the base sources directly via tsx inside that worktree. Mutants were applied one at a time in a third worktree (tmp/mut-tree) with exact-match-once pattern guards, restored after each run, and the three mutated files verified byte-identical to HEAD at the end; both scratch worktrees were removed after the cells were captured. Harnesses (manager-harness.mjs, base-contrast.mjs + base-contrast-base-arm.mts copied into the base worktree, base-contrast-head-arm.mjs, mutation-run.mjs, workflow-gates.mjs, print-ab-table.mjs, print-matrix.mjs) and per-arm vitest logs live in this artifact dir. Assertion counting: 2396 vitest A/B test results (base 1183 + head 1213) + 57 manager-harness checks + 20 contrast checks + 11 mutant-expectation assertions + 2 unmutated-control assertions + 11 gate assertions (ESLint gate + probe, actionlint gate + probe, tsc, workflow-gates ×6) = 2497; expected-red control cells count as passes; re-executions of already-counted tests (mutant runs, capture re-runs) are excluded to avoid double-counting.
Evidence images
Harness scripts and raw logs are in this artifact directory (also uploaded as workflow run artifacts).
— Qwen Code · sandboxed verification
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 29 passed · 0 failed · 29 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：29 通过 · 0 失败 · 29 总计 Verification reportPR 8890 Deep Verification — refactor(cli): Generalize the Conversations runtime foundationVerdict: 中文摘要
Scope selectionCentral claim: the Conversations runtime lifecycle is extracted from the Live namespace into a shared Secondary claims: (1) Budget allocation: A/B at the test-name level across both arms (~30%), one mock-free harness on the changed FS/registry surface (~15%), an 8-mutant matrix on the introduced guards (~30%), targeted gates: full Central claim + A/BEnvironment: CI merge-ref checkout (depth 2); base side is a scratch worktree at Witness:
The six new tests (3 lifecycle in One intentional semantic tightening, verified. Base adopted via Mock-free harness ( FindingsOrdered by severity. Neither is a merge condition; both are completeness reporting, and both concern code carried unchanged from base or defense-in-depth. 1. (Informational, pre-existing coverage gap) Publish-path
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-review of the delta since 783af45 (5 commits: f00873c, 76d57c6, 9f102ca, c666921, 9d08762 — all feedback-driven, no main merge). Approving.
Production delta (workspace-management.ts, +7/-30): owned-runtime publication switches from two-phase validation (pre-add validate + drain-stash post-add validatePublished with rollback) to a single validateBeforePublication hook that runs entirely before workspaceRegistry.add. The candidate never enters the registry unvalidated, closing wenshao's F4 transient visible-but-untrusted window by construction. The server.ts call site keeps the richer callback (manager admission validation + workspace revalidate) as the sole hook, so pre-publication validation is strictly stronger, not weaker. Duplicate-check + add remain serialized inside the topology lock (pinned by the new keeps a candidate unpublished and the topology lock free during final validation test), and disposal-on-failure is preserved for both the primary guard and validation rejection.
Invariants re-verified: admission thresholds incl. must not be primary guard intact and now mutation-pinned by rejects and disposes a primary owned-runtime candidate; cached-reuse assertExactRoot now asserted (F2); provenance/removability admission cases added at both existing-runtime and publication-candidate levels; manager drain/completeDrain rejection path pinned (no silent republish of a removed runtime); fail-closed directions unchanged. Residual validate→add TOCTOU is mitigated by per-use revalidation and was empirically exercised by wenshao's H4/H5 fail-closed arms on this exact head.
Feedback status: wenshao's 2026-08-13 real-daemon re-verification on 9d08762 confirms F1–F4 fixed (F2/F3 mutation-tested), base/head /capabilities parity byte-identical, single ACP child in every arm, 176/176 focused tests; F5 (silent fail-closed boot logging) remains a note-level follow-up. R6 items (reconciliation entry points via first ownership acquisition + exact delete retry; creation journal reconciliation) are reflected in the updated design doc, which is spec clarification for PR2+ only.
CI: all check runs on 9d08762 completed success/skipped — no red, no pending.
Already have 2 approves,3ks
|
Released in v0.21.12. |
4 participants
What this PR does
This PR implements the PR0 Conversations runtime foundation from the standalone daemon sessions design. It moves the existing Conversations workspace and source helpers out of the Live-specific namespace, introduces a daemon-internal manager that safely creates or adopts the single Conversations runtime, and keeps Live Voice as an adapter that transactionally installs its three bridge handlers.
Live and future standalone sessions can therefore share one Conversations runtime and one ACP bridge. Runtime publication remains lazy with respect to ACP process startup: ensuring the runtime registers the runtime and bridge but does not call ACP preheat, probe the provider, or initialize the native Host. The existing
live-conversationprovenance, storage layout, relocation token, public dependency field names, persisted metadata, capability behavior, and user-visible Live semantics remain unchanged.The dedicated Live Host workflow now also runs when the shared Conversations runtime code changes.
Why it's needed
The Conversations runtime lifecycle was previously embedded in Live Voice binding. Future projectless sessions need the same runtime without enabling Live or creating a second projectless runtime, while Live still needs strict ownership validation, one-flight publication, retryable failures, and clean handler teardown.
Separating runtime ownership from the Live adapter establishes that reusable boundary without adding standalone routes, source types, capabilities, SDK APIs, UI, virtual workspaces, per-session ACP children, or cross-daemon ownership.
Reviewer Test Plan
How to verify
Confirm concurrent runtime requests perform one publication, every later reuse revalidates the Conversations root, an existing active owned runtime is adopted, and invalid, draining, blocked, removed, or replaced runtimes fail closed without publishing a replacement or falling back to the primary runtime.
Confirm ensuring the runtime does not call ACP preheat or install Live handlers. With Live enabled, verify boot publication and a concurrent Host hello share the same publication; with Live disabled or ACP HTTP disabled, verify the Conversations root and runtime remain untouched.
Confirm a partial Live handler binding failure clears all three handlers, can retry without republishing the runtime, and invalidates the capability workspace cache as soon as publication succeeds. Confirm shutdown waits for in-flight Live binding, clears all handlers, and leaves runtime/bridge disposal to the existing workspace lifecycle.
Local verification completed with formatting, CLI lint, repository build, repository typecheck, 67 focused Conversations/Live unit tests, 17 Live server regression tests, 101 multi-workspace session tests, and 7 release-workflow tests. The full 933-test server file was also exercised repeatedly; all Live tests passed on every run, while unrelated existing tests showed nondeterministic shared-state failures when the entire file ran together.
Evidence (Before & After)
N/A — internal runtime refactor with no user-visible or TUI change.
Tested on
Environment (optional)
macOS local checkout with the repository Node.js/npm toolchain; tests used Vitest with coverage disabled to avoid the known shared coverage temporary-directory race.
Risk & Scope
Linked Issues
Refs #8908
中文说明
本 PR 做了什么
本 PR 实现 standalone daemon sessions 设计中的 PR0 Conversations runtime 基础设施。它将现有 Conversations workspace 和 source helper 从 Live 专属命名空间迁出，引入一个 daemon 内部 manager，用于安全地创建或采用唯一的 Conversations runtime，并保留 Live Voice 作为事务式安装三个 bridge handler 的 adapter。
这样 Live 与未来的 standalone 会话可以共用一个 Conversations runtime 和一个 ACP bridge。Runtime publication 对 ACP 进程启动仍保持懒加载：ensure 只注册 runtime 和 bridge，不调用 ACP preheat、不探测 provider、也不初始化原生 Host。现有
live-conversationprovenance、存储布局、relocation token、公开依赖字段名、持久化元数据、capability 行为以及 Live 的用户可见语义都保持不变。Live Host 专用 workflow 现在也会在共享 Conversations runtime 代码变更时运行。
为什么需要
Conversations runtime 生命周期此前嵌在 Live Voice 绑定逻辑中。未来的无项目会话需要在不启用 Live、也不创建第二套 projectless runtime 的情况下复用同一 runtime，同时 Live 仍需要严格的所有权校验、one-flight publication、可重试失败以及干净的 handler 清理。
本次拆分建立了这个可复用边界，同时不增加 standalone 路由、source type、capability、SDK API、UI、虚拟 workspace、每会话 ACP 子进程或跨 daemon ownership。
Reviewer 测试计划
如何验证
确认并发 runtime 请求只执行一次 publication，每次后续复用都会重新校验 Conversations root；确认合法且 active 的已有 owned runtime 会被采用；确认非法、draining、blocked、removed 或被替换的 runtime 会 fail closed，不发布替代 runtime，也不回退 primary runtime。
确认 ensure runtime 不调用 ACP preheat，也不安装 Live handler。Live 启用时，验证 boot publication 与并发 Host hello 共用同一次 publication；Live 禁用或 ACP HTTP 禁用时，验证 Conversations root 和 runtime 完全不被访问或创建。
确认 Live handler 部分绑定失败会清除全部三个 handler，后续可以在不重复发布 runtime 的情况下重试，并且 publication 一成功就会失效 capability workspace cache。确认 shutdown 会等待进行中的 Live binding、清除所有 handler，并继续由既有 workspace 生命周期负责 runtime/bridge 的释放。
本地已完成格式检查、CLI lint、全仓 build、全仓 typecheck、67 个 Conversations/Live 聚焦单测、17 个 Live server 回归测试、101 个 multi-workspace session 测试和 7 个 release-workflow 测试。还重复运行了完整的 933 项 server 测试文件；所有 Live 测试每次都通过，但整文件共同运行时，若干与本 PR 无关的既有测试出现了非确定性的共享状态失败。
证据（Before & After）
N/A —— 内部 runtime 重构，没有用户可见或 TUI 变化。
测试平台
环境（可选）
macOS 本地 checkout，使用仓库 Node.js/npm 工具链；Vitest 关闭 coverage，以避开已知的共享 coverage 临时目录竞争。
风险与范围
关联 Issue
Refs #8908