feat(serve): establish workspace runtime ownership - #7308
Conversation
🩺 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 |
|
Thanks for the PR! Template looks good ✓ Problem: this is architectural groundwork, not a bug fix. The motivation is clear and observed: Direction: aligned. The daemon's multi-workspace architecture needs a session-independent runtime owner, and this PR establishes that cleanly. The design doc ( Size: 3744 production logic lines + 4145 test lines + 1056 docs lines across 45 files. This PR spans 3 packages ( Approach: the architecture is sound — a 中文说明感谢贡献！ 模板完整 ✓ 问题：这是架构层面的基础工作，不是 bug 修复。动机清晰且已观测到： 方向：对齐。守护进程的多工作区架构需要一个独立于 Session 的 Runtime 所有者，本 PR 干净地建立了这一点。设计文档（ 规模：3744 行生产逻辑 + 4145 行测试 + 1056 行文档，跨 45 个文件。本 PR 跨越 3 个包（ 方案：架构合理—— — Qwen Code · qwen3.7-max Reviewed at |
Review: workspace runtime ownership (stack 1/4)OverviewMoves ACP lifecycle and capability state ownership from "last active session" to the registered workspace:
The layering is sound and the direction is right. Trust gating in particular is correct and fail-closed: Verified against the PR branch: Should fix before merge1. Undocumented breaking change to
|
Addendum: two
|
| site | reachable in production? |
|---|---|
bridge.ts:2002 — mcpAuthenticationCompleted notification handler |
No producer exists |
bridge.ts:7497 — non-pending success branch |
Only on success |
bridge.ts:2094 — .clear() on channel death / shutdown |
Only on teardown |
bridge.ts:3127 — status poll |
Requires server !== undefined && authenticationState !== 'pending' |
On the first row: grep -rn "authentication-completed\|mcpAuthenticationCompleted" across the repo returns the constant (status.ts:238), the consumer (bridgeClient.ts:1239), and two emissions, both in bridge.test.ts (:445, :480). packages/cli/src/acp-integration/acpAgent.ts has zero operationId references — the child is handed operationId at bridge.ts:7435 and ignores it entirely, and this PR doesn't touch acp-integration/. So bridge.test.ts:456 ("keeps an MCP authentication lease when the entry request fails before pending") passes by emitting a notification that nothing in production emits.
That leaves the status poll at :3127 as the only real release path for a failed authenticate — and the coordinator's own release loop cannot reach it. workspace-runtime-mcp-operations.ts:522-528:
const physicalPending = this.runtime.bridge.isWorkspaceMcpAuthenticationPending?.(operationId);
if (physicalPending === false) break;
if (physicalPending === true) {
await wait(MCP_POLL_INTERVAL_MS);
continue; // <-- the getWorkspaceMcpStatus call below is unreachable while the lease is held
}While the lease is held the loop continues before the status call, so it spins without ever performing the poll that would clear it. On the pending path monitorAuthentication polls status independently and breaks the cycle — but the entry-failure path (failAuthenticationWhenSafe → releaseAuthenticationWhenSafe) has no such poller.
Failure: POST .../runtime/mcp/:server/authenticate, the child rejects or the deadline elapses. The lease is retained; hasNoChannelWork() is false (bridge.ts:1491), so hasActiveWorkspaceWork() stays true, the coordinator reports 'active' and never 'idle', the idle reaper can never reclaim the workspace, and a 4 Hz busy loop runs per failed operation with releaseOperation() at the bottom never reached — leaking the coordinator's operation slot too.
Not strictly permanent: any unrelated client hitting GET /workspace/mcp triggers requestWorkspaceStatus → :3127 and clears it. But recovery depends on incidental external traffic, which isn't a lifecycle contract. Suggest a finally that releases the lease on entry failure, and reordering releaseAuthenticationWhenSafe so the status poll runs even when the lease reads pending.
Related, low severity: bridgeClient.ts:1237-1251 silently drops the notification unless params['v'] === 1 and early-returns either way. Fine today, but if that handler ever becomes the real release path, a producer omitting v leaks the lease with zero diagnostics — worth a writeStderrLine on the reject path.
B. killChannelWithLog is now unreachable under the default config
This sharpens item 1(b) — the default flip is not just resource retention, it removes the last self-healing path for a wedged child.
killChannelWithLog went from two callers to one:
OLD: :1463 await killChannelWithLog(ci, context) // the timeoutMs <= 0 immediate-kill branch
:1473 void killChannelWithLog(ci, 'idle timeout')
NEW: :1479 void killChannelWithLog(ci, 'idle timeout') // only caller
The surviving caller sits after startIdleTimer's if (timeoutMs === null) { cancelIdleTimer(); return; } guard, and null is now the default. So with no --channel-idle-timeout-ms set, nothing in the bridge can kill a channel except shutdown()/killAllSync(). The session-teardown paths that previously did (last session leaving, newSession failure on an empty channel, failed restore) are all deleted in this PR.
Failure: ci.connection.newSession(...) at :2303 is guarded only by withTimeout(initTimeoutMs) — it is not raced against getChannelClosedReject. If the child wedges (blocking prompt, stuck parse), the call times out, sessionRegistered stays false, and the finally at :2540 calls startIdleTimer — a no-op. channelInfo still points at the hung child, so every subsequent createSession → ensureChannel() returns that same child at :1890 and times out again. The workspace is permanently dead until daemon restart. Before this PR the first failure killed the channel and the next attempt cold-started.
This is the strongest argument for keeping channelIdleTimeoutMs: 0 working (item 1a) — under the new defaults, operators who relied on immediate reap lose both the reap and the crash-recovery behavior, and the flag value that would restore it now refuses to boot.
Also worth a look (not verified as deeply)
bridge.ts~14 sites of the formfinally { if (hasNoChannelWork(info)) startIdleTimer(info) }(:3148,:4534,:6087,:6113,:6139,:6163,:6314,:6490,:7525,:7566,:7607,:7702,:7765) don't checkchannelInfo === info, unliketrackWorkspacePhysicalRequestwhich guards withchannelInfo === ci && !ci.isDying(:1570). SinceidleTimeris a single module-scoped variable butstartIdleTimer(ci)closes over a specificci, a late-settling operation on a dead channel A cancancelIdleTimer()live channel B's timer and re-arm bound to A — leaking B. Only bites whenchannelIdleTimeoutMsis configured.bridge.ts:7838-7844— thecanceladded tokillSessionruns afternotifyAgentSessionClose, whose handler already callscancelPendingPrompt()and deletes the session (acpAgent.ts:3207/:3234).AcpAgent.cancelthen throwsSession not found, and becausecancelis an ACP notification the throw never reaches the bridge, so thetry/catchat:7839never fires. Harmless dead code, but it doesn't provide the protection it looks like it does.
Cleared on inspection: epoch assignment is sound (ensureChannel coalesces via inFlightChannelSpawn, so two channels can't claim the same epoch); trackWorkspacePhysicalRequest's counter pairs correctly with withWorkspaceControl (microtask ordering puts the physical decrement first, so no premature idle arm); getChannelClosedReject's cached rejection always gets a handler via Promise.race; shutdown/killAllSync still cancel the idle timer and clear auth timers; waitForWorkspacePhysicalRequests awaits never-rejecting promises.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built. Not reviewed: coverage — the plan could not be used (ENOENT: no such file or directory, open '/Users/jinye.djy/Projects/qwen-code/.qwen/tmp/qwen-review-pr-7308-fetch.json'), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (ENOENT: no such file or directory, open '/Users/jinye.djy/Projects/qwen-code/.qwen/tmp/qwen-review-pr-7308-fetch.json').
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] MCP auth timeout timer doesn't delete operation entry (bridge.ts:7471) — timer callback only deletes from workspaceMcpAuthenticationTimers, not workspaceMcpAuthenticationOperations, blocking hasNoChannelWork permanently when OAuth never completes and channelIdleTimeoutMs is configured
[Critical] Auth operation cleanup dropped discoveryState fallback (bridge.ts:3123) — server-absent-with-completed-discovery cleanup branch removed, operations for vanished servers persist forever blocking idle reap
[Critical] newSession failure self-healing removed (bridge.ts:~2300) — hung child on empty channel is no longer killed and replaced; with default sticky config, workspace is permanently dead until daemon restart
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: chunk 12, chunk 27, chunk 28, chunk 18, chunk 2, chunk 22, chunk 17, chunk 3, chunk 4, chunk 8, chunk 21, chunk 10, chunk 13, chunk 24, chunk 9, chunk 19, chunk 5, chunk 16, chunk 11, chunk 23, chunk 1, chunk 20, chunk 7, chunk 14, chunk 15, chunk 6, chunk 26, chunk 25 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Test coverage matrix (whole-diff), Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification, Invariant agent A: state, timers, collections — packages/acp-bridge/src/bridge.ts, Invariant agent B: counters, return values, error taxonomies — packages/acp-bridge/src/bridge.ts, Invariant agent C: config fields, early returns — packages/acp-bridge/src/bridge.ts — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries. Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it. [Critical] SDK type channelIdleTimeoutMs: number in packages/sdk-typescript/src/daemon/types.ts:458 does not match bridge's number | null. When daemon runs without --channel-idle-timeout-ms (the new default), limits.channelIdleTimeoutMs is null at runtime despite TypeScript declaring number. SDK consumers using strict null checks or serialization round-trips would fail.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: Review feedback addressedCritical fixes1. Fixed. Two changes:
Added a regression test: "releases the OAuth lane and barrier on dispose even while the channel is live" — verifies that a second workspace can authenticate immediately after the first is disposed with a live channel. 2. SDK type Fixed. Changed Suggestions addressed3. OAuth tests lack Fixed. All 8 OAuth-related tests now use 4. Design doc activation union type lacks semantic definitions (@qwen-code-ci-bot, Fixed. Added inline comments defining each value of the Already fixed in 3925389 (confirmed)
Declined with reason
ConflictNo conflict ( Verification
中文说明已处理的评审反馈关键修复1. 已修复。两处更改：
新增回归测试："releases the OAuth lane and barrier on dispose even while the channel is live"——验证在通道仍然存活时 dispose 第一个工作区后，第二个工作区可以立即进行认证。 2. SDK 类型 已修复。将 SDK 的 已处理的建议3. OAuth 测试缺少 已修复。所有 8 个 OAuth 相关测试现在使用 4. 设计文档 activation 联合类型缺少语义定义（@qwen-code-ci-bot， 已修复。为 已在 3925389 中修复（已确认）
附理由拒绝
冲突无冲突（ 验证
Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
Review — workspace runtime ownership (stack 1/4)Thanks for this. The runtime-ownership model reads well, and the test suite is genuinely strong — the coordinator's cases around epoch changes, stale projection, drain-rollback and OAuth-lane serialization pin down exactly the right invariants. I reviewed the full diff plus the surrounding bridge context. One blocking issue, a few medium correctness/compat items, and some low-severity notes. 🔴 Blocking — reentrancy deadlock in the capability physical laneIn
Impact: the mcp lane is wedged forever — every later MCP op (reconcile / mutation / prepare / auth, which all share the lane) queues behind the dead task; The 🟠 Medium
🟡 Low / notes
Positives
Net: the deadlock is the one I'd block on. The medium items are worth addressing (or explicitly deferring within the stack, noted in the PR) before merge. 🤖 Reviewed with Claude Code · model: Opus 4.8 (1M) |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: chunk 1, chunk 2, chunk 3, chunk 4, chunk 5, chunk 6, chunk 7, chunk 8, chunk 9, chunk 10, chunk 11, chunk 12, chunk 13, chunk 14, chunk 15, chunk 16, chunk 17, chunk 18, chunk 19, chunk 20, chunk 21, chunk 22, chunk 23, chunk 24, chunk 25, chunk 26, chunk 27, chunk 28 — no agent reported covering these; nobody read them. Not reviewed: every dimension — none of the 31 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries. Not reviewed: verification — the review posts findings, but no verifier was launched with a prompt this skill builds — they were ruled on, if at all, without the verdict bar its brief carries.
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: Review feedback addressed🔴 Blocking — reentrancy deadlock in the capability physical lane (@wenshao)Decision: Fixed (Required — correctness bug / blocking). Root cause: Fix: Detached Files: Test: Added regression test [Critical]
|
| Feedback | Reason |
|---|---|
| 🟠 Pending OAuth blocks other MCP ops and can pin the runtime (@wenshao) | Complex design change requiring bounded wait / fast-fail for non-auth ops and lease release on abandoned auth. Out of scope for a minimal review-fix commit; should be a follow-up PR. |
🟠 Compat: channelIdleTimeoutMs semantics changed (@wenshao) |
Intentional per the PR body. The docs fix above addresses the 0 value contradiction. Changelog / migration notes are a release-process concern, not a code fix. |
| 🟡 Low / notes (@wenshao) | All acknowledged as latent smells or intentional behavior changes. None are live bugs requiring immediate fixes. The sessionSpawnResetPending sticky flag (the one actionable item) is fixed above. |
| CI failure: Test (ubuntu-latest Node 22.x) | Could not reproduce locally — all focused test suites pass (66 coordinator, 432 bridge, 762 server, 249 serve routes, 13 workspace-runtime routes). The failure may be flaky or environment-specific; the workflow should re-run CI on the new commit. |
Verification
| Check | Result |
|---|---|
npx tsc --noEmit (cli) |
✅ Pass |
npx tsc --noEmit (acp-bridge) |
✅ Pass |
npx eslint (all 5 changed source/test files) |
✅ Pass |
vitest run workspace-runtime-coordinator.test.ts |
✅ 66/66 pass |
vitest run bridge.test.ts |
✅ 432/432 pass |
vitest run server.test.ts |
✅ 762/762 pass |
vitest run workspace-runtime.test.ts |
✅ 13/13 pass |
vitest run workspace-management.test.ts + run-qwen-serve.test.ts |
✅ 249/249 pass |
中文说明
已处理的评审反馈
🔴 阻塞 — 能力物理通道中的重入死锁（@wenshao）
决定： 已修复（必需 — 正确性缺陷 / 阻塞项）。
根因： reconcileCapability 和 executeMcpRuntimeMutationInPhysicalLane 都在 runInCapabilityPhysicalLane(capability, …) 回调内部 await 了 resumeCapabilityInBackground(...)。resumeCapabilityInBackground 循环调用 prepareCapability(...)，而后者会重新获取同一物理通道。由于外层通道的 tail 只在外层 body 返回后才 resolve，而外层 body 又阻塞在 resume 上，形成了循环等待 → 当运行时 epoch 在操作中途翻转时产生永久挂起。
修复： 在两个位置将 resumeCapabilityInBackground 从物理通道中解耦，使用 void … .catch(() => undefined)，与 prepare() 中已有的模式一致（在通道任务 resolve 之后的 .then() 中调度 resume）。外层通道 body 现在立即返回，释放通道，使后台 resume 可以重新获取通道而不会死锁。
文件： packages/cli/src/serve/workspace-runtime-coordinator.ts（2 处）
测试： 新增回归测试 does not deadlock when epoch flips during a physical lane reconciliation — 在物理通道阻塞于 prepareMcp 时翻转 epoch，然后断言协调过程能够完成（5 秒超时保护）。
[Critical] sessionSpawnResetPending 粘滞标志（@qwen-code-ci-bot）
决定： 已修复（必需 — 正确性缺陷）。
根因： sessionSpawnResetPending 在 newSession 超时时被设为 true，但在后续成功的 newSession 后从未清除。恢复的通道可能在下一次空闲时被粘滞标志强制杀死。
修复： 在成功的 newSession try-catch 块之后、延迟关闭重检之前，添加 ci.sessionSpawnResetPending = false;。成功的 spawn 现在会推翻之前的超时信号。
文件： packages/acp-bridge/src/bridge.ts
[Suggestion] dispose() 未清除跟踪映射（@doudouOUC / @qwen-code-ci-bot）
决定： 已修复（可选 — 有价值，在范围内）。
修复： 在 dispose() 中添加 this.inFlight.clear()、this.backgroundResume.clear() 和 this.capabilityPhysicalTail.clear()。在途 promise 仍会自行 settle（触发 assertAcceptingWork 并失败）；清除映射只是阻止 hasActiveWork() 在已销毁的运行时上报告误报。
文件： packages/cli/src/serve/workspace-runtime-coordinator.ts
[Suggestion] 文档中 channelIdleTimeoutMs: 0 语义矛盾（@qwen-code-ci-bot）
决定： 已修复（可选 — 文档一致性）。
修复： 重写第 16 节第 9 项以匹配三态模型：null → 不自动回收；0 → 启动时明确失败（TypeError）；极小正值 → 正常空闲回收。
文件： docs/design/workspace-runtime-architecture.md
[Suggestion] killSession 的 connection.cancel() 缺少测试（@qwen-code-ci-bot）
决定： 已修复（可选 — 测试覆盖）。
修复： 新增测试 killSession calls connection.cancel with the killed sessionId — 创建会话、启动 prompt、杀死会话，并断言 connection.cancel 以被杀死的 sessionId 被调用。
文件： packages/acp-bridge/src/bridge.test.ts
🟠 中等 — 未映射的 WorkspaceRuntimeStillStartingError → 500 空响应体（@wenshao）
决定： 已修复（必需 — 正确性缺陷）。
根因： WorkspaceRuntimeStillStartingError 是一个无消息、未导出的 Error 子类，在 sendBridgeError 中没有对应的处理分支。客户端收到的是 500 和空的 error 字段，而非预期的 503 和有意义的消息。
修复： 导出该类，添加构造函数消息（'Workspace runtime is still starting'），并在 sendBridgeError 中添加映射分支，返回 503、Retry-After: 5 和 code: 'runtime_still_starting'。子类 WorkspaceRuntimeEpochChangedError 也受益（继承消息并匹配同一 503 分支）。
文件： packages/cli/src/serve/workspace-runtime-coordinator.ts、packages/cli/src/serve/server/error-response.ts
延迟处理项
| 反馈 | 原因 |
|---|---|
| 🟠 待处理 OAuth 阻塞其他 MCP 操作并可能固定运行时（@wenshao） | 复杂的设计变更，需要对非认证操作添加有界等待/快速失败，以及在废弃认证时释放租约。超出最小评审修复提交的范围；应作为后续 PR 处理。 |
🟠 兼容性：channelIdleTimeoutMs 语义变更（@wenshao） |
按 PR 描述为有意变更。上述文档修复已解决 0 值的矛盾。变更日志/迁移说明属于发布流程事项，非代码修复。 |
| 🟡 低优先级 / 备注（@wenshao） | 均确认为潜在异味或有意的行为变更。无需要立即修复的活跃缺陷。sessionSpawnResetPending 粘滞标志（唯一可操作项）已在上方修复。 |
| CI 失败：Test (ubuntu-latest Node 22.x) | 本地无法复现 — 所有聚焦测试套件均通过（66 个协调器、432 个桥接、762 个服务器、249 个 serve 路由、13 个 workspace-runtime 路由）。该失败可能是 flaky 或环境特定的；工作流应在新提交上重新运行 CI。 |
验证结果
| 检查项 | 结果 |
|---|---|
npx tsc --noEmit（cli） |
✅ 通过 |
npx tsc --noEmit（acp-bridge） |
✅ 通过 |
npx eslint（所有 5 个变更的源文件/测试文件） |
✅ 通过 |
vitest run workspace-runtime-coordinator.test.ts |
✅ 66/66 通过 |
vitest run bridge.test.ts |
✅ 432/432 通过 |
vitest run server.test.ts |
✅ 762/762 通过 |
vitest run workspace-runtime.test.ts |
✅ 13/13 通过 |
vitest run workspace-management.test.ts + run-qwen-serve.test.ts |
✅ 249/249 通过 |
Base-conflict check: no conflict with main.
Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human.
🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview
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. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Unresolved, please confirm: [Critical] MCP auth operation entry leak on withTimeout throw (bridge.ts:7526) — author argues intentional (no OAuth cancellation contract); recovery via channel exit and status polling; debated across multiple review rounds without consensus Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen3.7-max via Qwen Code /review
|
已按本轮 re-review 的 3 个合入前问题修复并推送
新增回归覆盖了已过期 deadline、JSON-RPC 错误透传、以及 hung workspace physical request + hung restore 后替换 channel 的场景。 验证：
本次未处理评论中的 Minor/后续建议，避免在已多轮 review 的基础 PR 中继续扩大范围。 |
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. 3 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
ytahdn
removed
the
autofix/takeover
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts, but the head branch changed while resolving, so the update was not pushed. Re-run /resolve. The resolved diff is attached as the Merge Resolution Summary — PR #7308Root causeMain advanced on two independent fronts that collided with this PR's workspace runtime teardown hardening:
Textual or semantic
const hadActivePrompt = entry.promptActive;
try {
await notifyAgentSessionClose(entry, ci, 'killSession', {
throwOnFailure: true,
timeoutMs: initTimeoutMs,
});
} catch (error) {
if (ci) {
await killChannelWithLog(ci, `force kill session ...`);
return true;
}
entry.closing = false;
throw error;
}
if (hadActivePrompt) {
entry.promptActive = false;
activePromptCounter--;
touchActivity();
}
// ... state cleanup, channel detach ...
if (hadActivePrompt) {
void entry.connection.cancel({ sessionId }).catch(() => {});
}
What is load-bearing
What I could not verify
中文说明合并冲突解决总结 — PR #7308根因main 分支在两个独立方向上与本 PR 的工作区运行时拆卸硬化产生了冲突：
冲突性质
关键约束
未能验证
|
|
Pushed the conflict resolution and the two requested fixes:
Verification completed:
The full CLI suite was stopped at the author request after it had already confirmed the previous CI failure is fixed; under local high parallel load it also produced unrelated UI timeout flakes. No E2E was run. |
Code Review — #7308
|
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Requesting changes for one blocking process-global OAuth serialization gap.
The workspace/runtime ownership and fail-closed routing are substantially improved at this head, and the previously reported nullable-type, path-disclosure, conflict-mapping, and drain-cleanup issues appear resolved.
The blocker is that the new daemon-wide MCP authentication lane is not shared by the still-mounted legacy MCP mutation routes. This leaves a mixed-route, multi-workspace path that can start two ACP OAuth callback servers concurrently.
Non-blocking follow-ups: preserve X-Qwen-Client-Id on the new runtime MCP mutations; trim unused coordinator APIs until their production consumers land; and add an E2E covering lazy runtime startup, configured idle reaping, OAuth safe-drain, and mixed legacy/runtime routing. The PR currently has no dedicated E2E report and the integration job was skipped.
Verification at 3a3bf9c: npm run build, npm run typecheck, npm run lint, git diff --check, the ACP bridge tests, and the affected CLI runtime/ACP/serve tests passed locally. The Ubuntu Node 22, Serve A/B, and Web Shell smoke checks are also green.
| return true; | ||
| } | ||
|
|
||
| setExtensionsDesiredGeneration(generation: number): void { |
There was a problem hiding this comment.
[Suggestion] Defer unused coordinator APIs until their consumers land
The extensions generation/reconciliation methods beginning here, along with the management-operation helpers, have no production callers outside this class at this head. Keeping an unconsumed state-machine surface in this already large core coordinator makes the current ownership change harder to prove and maintain. Under the repository simplicity rule, please introduce these methods with the follow-up that actually calls them, or remove the unused public surface from this PR.
|
Triage re-run completed without a new review. The stage comments above were updated with the latest result. View workflow run. |
|
Qwen Code review timed out. Qwen review timed out after 300 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
Runtime verification — local A/B on the real daemon (merge reference)Following my code review above, I built both the PR head (
Results
Claim C is the crux of the PR — a registered workspace is prepared without a session, reports one Verified re #3 (divergent messages)Both validation sites are reachable and emit different text for the same Method & independent test re-run
Verdict: the runtime behavior matches the PR body and the docs exactly. No new blockers from this pass — the outstanding items remain the ones from my review (release-note the default-idle flip #1 + the SDK 🇨🇳 中文版运行时验证 —— 真实 daemon 的本地 A/B（合并参考）在前面的代码评审基础上，我本地分别构建了 PR head（ 结果
Claim C 是本 PR 的核心：已注册工作区在没有 session 的情况下即可完成准备，报告单一 方法与独立测试复跑
结论：运行时行为与 PR 描述、文档完全一致，本轮未发现新的阻塞项。待办仍是评审中的那几条：为默认空闲翻转（#1）与 SDK |
|
Follow-up to the A/B result: commit |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-reviewed at ce637fc829996dbedac9500c7ce7faa1c14c7fd9. The previous mixed-route OAuth serialization and runtime MCP client-id findings are resolved by moving the MCP runtime routes to #7309. Requesting changes for the current ACP lifecycle regression and red CI.
Verification on this head: the Ubuntu Node 22 job fails; locally bridge.test.ts reports 429 passed / 18 failed plus one unhandled rejection. Failures cover sessionless MCP management, Catalog/control completion, restore/newSession recovery, and last-session cleanup. A focused regression also confirms that default/explicit-zero preheat() returns after killing its channel. Build/bundle, typecheck, lint, diff check, and the affected CLI tests (364/364) pass.
Please fix the explicit-preheat regression and reconcile the 18 bridge failures against the intended compatibility contract rather than only changing expectations. The existing unused-coordinator-API thread remains open, so I did not duplicate it. Integration/E2E validation is still skipped.
| if (idleMs > 0 && hasNoChannelWork(ci)) { | ||
| await startIdleTimer(ci); | ||
| if (hasNoChannelWork(ci)) { | ||
| startIdleTimer(ci); |
There was a problem hiding this comment.
[Critical] Preserve explicit preheat under the compatibility zero timeout
The restored default maps an omitted channelIdleTimeoutMs to 0. This call therefore immediately reaches startIdleTimer zero branch, where killChannelWithLog marks the new channel isDying before preheat() resolves. POST /workspace/acp/preheat then observes isChannelLive() === false and returns ready: false / ACP preheat did not produce a live channel. A focused regression on this head confirms await bridge.preheat() leaves handle.killed === true. The base implementation only armed the preheat idle timer for a positive timeout; please preserve that compatibility special-case and add omitted/explicit-zero coverage.
| daemon-local config GET 仍可读，global config owner 不受 primary trust 影响。 | ||
| 8. **SDK transport**：REST、ACP HTTP/WS 模式下 Workspace client 都不会把 daemon | ||
| runtime 路由误发为 ACP method。 | ||
| 9. **连续 lease**：未配置 idle timeout（`null`）时不自动回收；`0` 配置明确失败（启动时 `TypeError`）；显式极小正值时，preheat、 |
There was a problem hiding this comment.
[Suggestion] Update this invariant to the restored timeout contract
This still says omitted timeout disables reaping and explicit 0 fails startup, but the current code and PR description now define omitted and 0 as immediate cleanup. Please update the design invariant so reviewers and the follow-up stack do not implement against the superseded semantics.
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. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Unresolved, please confirm: [Critical] bridge.ts:7611 — MCP auth operation entry on withTimeout throw: maintainer-acknowledged design decision (no OAuth cancellation contract); debated across multiple review rounds without consensus Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies. Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
— qwen3.7-max via Qwen Code /review
| Omitting that channel option keeps the Workspace Runtime live; configured | ||
| values must be positive. |
There was a problem hiding this comment.
[Suggestion] "Configured values must be positive" contradicts the validation in run-qwen-serve.ts:2376-2383 which accepts 0 (error message: "Must be a non-negative integer (milliseconds, 0 = immediate kill)"). The edge-case table at line 436 of this same file also says "an unset or zero timeout reaps immediately." The word "positive" excludes zero; the code and the rest of the doc include it.
| Omitting that channel option keeps the Workspace Runtime live; configured | |
| values must be positive. | |
| Omitting that channel option keeps the Workspace Runtime live; configured | |
| values must be non-negative. A value of `0` reaps immediately. |
— qwen3.7-max via Qwen Code /review
| # 12. Opt into compatibility Workspace Runtime auto-reaping | ||
| qwen serve --channel-idle-timeout-ms 60000 |
There was a problem hiding this comment.
[Suggestion] "Opt into compatibility Workspace Runtime auto-reaping" is not a term used anywhere else in the codebase or docs, and the reader cannot determine what the flag does or why to set it. The table entry for the same flag later in this file says "ACP child auto-reap delay after all Session and management work drains" — clear and self-explanatory.
| # 12. Opt into compatibility Workspace Runtime auto-reaping | |
| qwen serve --channel-idle-timeout-ms 60000 | |
| # 12. Set ACP child auto-reap delay to 60s after work drains | |
| qwen serve --channel-idle-timeout-ms 60000 |
— qwen3.7-max via Qwen Code /review
| export function normalizeWorkspaceRuntimeTimeout( | ||
| value: unknown, | ||
| ): number | undefined { | ||
| if (value === undefined) return DEFAULT_PREPARE_TIMEOUT_MS; |
There was a problem hiding this comment.
[Suggestion] This exported function has zero read sites in the entire codebase — dead public API surface. ensure() and prepare() accept timeoutMs without validation through this function. Future developers seeing this export may assume timeout configuration is wired through it.
Suggested fix: either wire it into the caller path that supplies timeoutMs to ensure()/prepare(), or remove the export and inline the validation where needed.
— qwen3.7-max via Qwen Code /review
🔍 Local Verification Report — PR #7308Branch: Test Results
tmux CLI Startup✅ 构建 core 后 CLI 正常启动（v0.20.0） Architecture Review论点: Workspace 是运行时、隔离和管理边界；Session 只是 Workspace Runtime 中的消费者。这是 daemon 架构从 Session-centric 到 Workspace-runtime-centric 的根本性迁移。 论据:
论证:
Verdict✅ 1030/1030 测试全部通过，CLI 正常启动。架构设计精密，不变量清晰，建议合并。 这是 workspace runtime 系列（#7308 → #7309 → #7310 → #7311）的基础 PR，后续 PR 在此所有权模型上构建 MCP/Extension/Skill 管理。 Verified locally: unit tests + CLI startup on macOS |
6 participants
Summary
This PR introduces workspace-owned runtime coordination for
qwen serve. ACP lifecycle and capability state now belong to the registered workspace instead of the last active session, with explicit runtime status, startup, reconciliation, and idle cleanup behavior.The following lifecycle changes are intentional:
channelIdleTimeoutMsor passing explicit0reaps the ACP child immediately after all workspace runtime work drains.--channel-idle-timeout-mswindow. Runtime-control leases protect initialization and each request from mid-operation cleanup; reaping begins only after the request completes and all physical work drains.qwen serveno longer eagerly preheats the primary ACP child by default. The first session or runtime-management request starts it, so the first request may pay the cold-start cost.This is stack 1/4 and is based on
main.Why it's needed
Extensions, MCP, Skills, and Tools management need a stable owner even when no chat session exists. The workspace runtime provides that owner while sessions become consumers of the reusable ACP-backed runtime.
Reviewer Test Plan
How to verify
Confirm a registered workspace can be prepared without creating a session and reports one runtime epoch with per-capability state. Verify an omitted or zero channel idle timeout reaps the ACP child after runtime work drains, while a positive timeout reuses the same child for later sessions and management requests.
Evidence (Before & After)
Before: ACP lifetime followed session attachment. After: ACP-backed capability state is owned by the workspace runtime.
Tested on
Environment (optional)
Local npm workspace. ACP bridge and CLI type checks passed; targeted runtime, bridge, management, and serve unit tests passed. End-to-end validation was not run.
Risk & Scope
0both mean immediate cleanup, and positive values delay cleanup. Persisted feature configuration formats do not change.Linked Issues
Stack 1/4: #7308 Runtime foundation → #7309 MCP → #7310 Extensions → #7311 Skills.
中文说明
摘要
本 PR 为
qwen serve引入工作区持有的 Runtime 协调层。ACP 生命周期和能力状态改由已注册工作区持有，而不是由最后一个活动 Session 持有，并提供明确的 Runtime 状态、启动、协调和空闲回收行为。以下生命周期变化均为有意设计：
channelIdleTimeoutMs或显式传入0时，工作区 Runtime 的全部任务排空后立即回收 ACP 子进程。--channel-idle-timeout-ms。runtime-control lease 会覆盖初始化和整次请求，只有请求完成且所有物理任务排空后才会开始回收。qwen serve默认不再预热主工作区 ACP。第一个 Session 或 Runtime 管理请求会按需启动它，因此首次请求可能承担冷启动耗时。这是堆叠 PR 的第 1/4 个，基于
main。为什么需要
拓展、MCP、Skills 和 Tools 管理需要在没有聊天 Session 时仍有稳定的状态所有者。Workspace Runtime 提供该所有权，Session 则变成可复用 ACP Runtime 的消费者。
审查测试计划
如何验证
确认已注册工作区无需创建 Session 即可准备服务，并能报告 Runtime epoch 和各能力状态。验证未配置或配置为
0时在 Runtime 任务排空后立即回收 ACP 子进程；配置正数时，后续 Session 和管理请求会复用同一个子进程。前后对比证据
改动前：ACP 生命周期跟随 Session。改动后：ACP 支撑的能力状态由 Workspace Runtime 持有。
测试平台
环境（可选）
本地 npm workspace。ACP bridge 和 CLI 类型检查通过，针对 Runtime、bridge、管理和 serve 的单元测试通过；未执行端到端验证。
风险与范围
0都表示立即回收，正数表示延迟回收。已有功能配置格式不变。关联事项
堆叠 PR 第 1/4 个：#7308 Runtime 基础架构 → #7309 MCP → #7310 拓展 → #7311 Skills。