feat(serve): Hot-reload workspace trust changes - #7268
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 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 3 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Document the trust hot-reload capability and reuse the daemon environment fallback so the serve process environment guard remains satisfied. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Automated CI follow-up for commit
Verification: |
|
Thanks for the PR! Template looks good ✓ Problem: This is a well-documented, observed limitation — workspace trust is evaluated once at runtime construction and never re-evaluated, so policy changes (user settings, system defaults, IDE trust, trusted-folder writes) require a daemon restart. The linked RFC (#6378) has been through multiple rounds of maintainer review with extensive scope refinement, and this PR implements the trust hot-reload phase. The problem is real and the evidence is clear. Direction: Aligned with the daemon's multi-workspace roadmap. The RFC has been deliberately scoped down through maintainer feedback, and trust hot-reload is a prerequisite for safe multi-workspace semantics — a stale trusted runtime after revocation is both confusing and a security concern. CHANGELOG has no direct reference to trust hot-reload, but the daemon/multi-workspace area is actively tracked (#3803, #4514, #5976). Size: This PR touches core paths across 4 packages (cli, core, sdk-typescript, acp-bridge) with ~6,046 production logic lines (additions + deletions, excluding tests) and ~5,492 test lines across 125 files. Core-path production changes are small (~49 lines in Approach: The architecture is well-designed — semantic policy snapshots with hash-based change detection, a poller + event-driven monitor, generation guards for in-flight operation invalidation, a reconciler with drain/dispose/fallback lifecycle, and route-level 503s during transitions. The v2 trust API is additive (v1 preserved), and the SDK types are clean. The design doc clearly documents 6 security invariants. The scope feels justified by the security requirements — trust revocation must close the old generation before any asynchronous cleanup, which touches every workspace-scoped route. No unrelated changes or drive-by refactors detected. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个有充分文档记录的已观测限制——workspace trust 只在 runtime 构建时评估一次，之后不会重新评估，因此策略变更（用户设置、系统默认值、IDE trust、trusted-folder 写入）需要重启 daemon。关联的 RFC（#6378）经过多轮 maintainer 审查和范围细化，本 PR 实现了 trust hot-reload 阶段。问题是真实的，证据清晰。 方向：与 daemon 多 workspace 路线图一致。RFC 经过 maintainer 反馈后有意缩小范围，trust hot-reload 是安全多 workspace 语义的前提——撤销 trust 后仍保留过期的 trusted runtime 既令人困惑也存在安全隐患。CHANGELOG 没有直接提及 trust hot-reload，但 daemon/多 workspace 领域正在积极跟踪中（#3803、#4514、#5976）。 规模：本 PR 跨 4 个包（cli、core、sdk-typescript、acp-bridge）触及核心路径，约 6,046 行生产逻辑代码（增删合计，不含测试）和 约 5,492 行测试代码，共 125 个文件。核心路径生产改动较小（ 方案：架构设计良好——语义化策略快照（基于哈希的变更检测）、轮询 + 事件驱动监控、generation guard（用于进行中操作的失效）、带 drain/dispose/fallback 生命周期的协调器、以及过渡期间路由级 503。v2 trust API 是新增的（v1 保留），SDK 类型清晰。设计文档清楚记录了 6 个安全不变量。范围由安全需求证明是合理的。未检测到无关改动或顺手重构。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Thanks — partially agreed. The size warrants maintainer awareness, but I’m deferring a split for this PR: the policy snapshot, reconciliation, generation guard, runtime routing, and status protocol jointly enforce one fail-closed trust boundary. Landing only some layers would leave stale-runtime consumers or require temporary compatibility contracts that the next slice removes. Since implementation and verification are complete, I’ll keep the scope stable and limit subsequent changes to correctness, security, data-loss, or regression fixes; maintainers can still request a different landing strategy. |
Code ReviewIndependent proposal: For trust hot-reload, I would add: (1) a side-effect-free policy snapshot reader with hash-based revision tracking, (2) a poller/watcher for policy file changes with event-driven triggers for IDE and same-process writes, (3) a close-once generation guard to invalidate in-flight operations, (4) a registry entry → generation model separating stable workspace identity from mutable runtime state, (5) a reconciler that plans replacements, drains old runtimes concurrently for trust decreases, then builds and activates new ones with a fallback to untrusted on construction failure, (6) route-level 503 + Retry-After during transitions, and (7) an additive v2 status API reporting configured vs effective trust and reconciliation state. Comparison: The PR matches this proposal almost exactly, and goes further in several areas: the No critical blockers found. The route changes are mechanical and consistent — each workspace-qualified route resolves the entry via The registry refactoring is well-structured: One observation (non-blocking): the Delta since last review ( Files changed (30 of 125 shown)
CI Test EvidenceFetched via the GitHub API for commit
The ubuntu test suite, Serve A/B, web-shell E2E smoke, and precheck all passed on the current head. macOS, Windows, and integration tests were skipped (not failed). Not verified: Windows and Linux platform-specific behavior (author tested on macOS only). The 中文说明代码审查独立方案： 对于 trust hot-reload，我会添加：(1) 无副作用的策略快照读取器（基于哈希的 revision 跟踪），(2) 轮询 + 事件驱动的策略文件变更监控，(3) 一次性关闭的 generation guard，(4) registry entry → generation 模型，(5) 协调器（规划替换、并发 drain、构建并激活新 runtime，构建失败时回退到 untrusted），(6) 过渡期间路由级 503 + Retry-After，(7) 新增 v2 状态 API。 对比： PR 几乎完全匹配此方案，并在多个方面更进一步。安全不变量在代码中得到强制执行。未发现关键阻塞问题。路由变更机械化且一致。v2 API 正确门控。 一个观察（非阻塞）：协调器 上次审查以来的变更（ CI 测试证据通过 GitHub API 获取 commit 未验证：Windows 和 Linux 平台特定行为。maintainer 可以触发 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review and the architecture is sound, but ~6,046 production lines of core trust/daemon infrastructure from a fork needs a maintainer's sign-off. This is a well-executed PR. The architecture matches what I'd propose independently — semantic policy snapshots, generation guards, a reconciler with drain/dispose/fallback lifecycle, and route-level fail-closed behavior. The security invariants are documented in the design doc and enforced in code: generation guards close before the first async drain step on trust decrease, closed guards never reopen, failed revocations never restore the previous trusted runtime, and no workspace-scoped route falls back to the primary runtime. The The v2 trust API is additive and backward-compatible, the SDK types are well-structured, and the route changes are mechanical and consistent. CI passes on ubuntu (test suite, Serve A/B, web-shell E2E, precheck) at the current head. The test coverage is substantial at ~5,492 lines across the new policy, monitor, reconciler, and route paths. The four commits since the last review round are incremental hardening fixes — closing generation gaps, preserving error semantics, and simplifying routes — not architectural changes. The reason this is 3/5 and not higher is purely the Stage 0 escalation: this is a cross-repository fork PR touching core trust and daemon infrastructure at scale (125 files, ~6,046 production lines across 4 packages). The code quality is high and I found no critical blockers, but the blast radius of a missed downstream consumer or a platform-specific watcher difference is significant enough that a maintainer should review the route-ownership changes and the reconciler's failure paths before merge. The author flagged this risk themselves and requested focused route-ownership review, which I think is the right call. Deferring to @wenshao for the final call on this one. 中文说明置信度：3/5 —— 审查干净且架构合理，但来自 fork 的约 6,046 行核心 trust/daemon 基础设施代码需要 maintainer 签核。 这是一个执行良好的 PR。架构与我独立提出的方案一致——语义化策略快照、generation guard、带 drain/dispose/fallback 生命周期的协调器、以及路由级 fail-closed 行为。安全不变量在设计文档中有记录并在代码中得到强制执行。 v2 trust API 是新增且向后兼容的，SDK 类型结构良好，路由变更机械化且一致。CI 在当前 head 的 ubuntu 上通过（测试套件、Serve A/B、web-shell E2E、precheck）。测试覆盖充分，约 5,492 行。上次审查后的四个 commit 是增量加固修复，非架构变更。 评为 3/5 而非更高的原因纯粹是 Stage 0 升级：这是一个跨仓库 fork PR，大规模触及核心 trust 和 daemon 基础设施（125 个文件，跨 4 个包的约 6,046 行生产代码）。代码质量高且未发现关键阻塞问题，但遗漏下游消费者或平台特定 watcher 差异的影响范围足够大，maintainer 应在合并前审查路由所有权变更和协调器的失败路径。 转交 @wenshao 做最终决定。 — Qwen Code · qwen3.8-max-preview Reviewed at |
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 reviewed: chunk agents 1-44 — launched with custom prompts due to working_dir/run_in_background incompatibility; core production code reviewed directly by orchestrator. Not reviewed: test-matrix — agent did not complete before review composition. Not reviewed: removed-behavior — agent did not complete before review composition. Not reviewed: cross-file-tracer — agent did not complete before review composition. Not reviewed: invariant agents — launched with custom prompts; workspace-registry.ts and workspace-trust-reconciler.ts reviewed directly by orchestrator. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, 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, chunk 29, chunk 30, chunk 31, chunk 32, chunk 33, chunk 34, chunk 35, chunk 36, chunk 37, chunk 38, chunk 39, chunk 40, chunk 41, chunk 42, chunk 43, chunk 44, 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/cli/src/serve/run-qwen-serve.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent A: state, timers, collections — packages/cli/src/serve/workspace-registry.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/workspace-registry.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/workspace-registry.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 — 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
|
Automated review follow-up for commit
Verification: focused route and reconciliation tests passed, changed files passed ESLint, and |
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. Not reviewed: chunk agents — launched with paraphrased prompts due to working_dir/run_in_background incompatibility; all 59 agents read their briefs and diff chunks substantively. Not reviewed: chunk 3, chunk 40, chunk 41, chunk 22, chunk 36, chunk 35, chunk 19, chunk 43, chunk 17, chunk 34, chunk 44, chunk 12, chunk 27, chunk 42, chunk 29, chunk 38, chunk 6, chunk 15, chunk 10, chunk 7, chunk 4, chunk 31, chunk 2, chunk 30, chunk 16, chunk 1, chunk 37, chunk 18, chunk 33, chunk 9, chunk 25, chunk 24, chunk 13, chunk 8, chunk 20, chunk 21, chunk 26, chunk 14, chunk 23, chunk 11, chunk 32, chunk 28, chunk 39, chunk 5 — launched with a prompt that is not the one the CLI built. Not reviewed: 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/cli/src/serve/run-qwen-serve.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent A: state, timers, collections — packages/cli/src/serve/workspace-registry.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/workspace-registry.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/workspace-registry.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 — 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 batch in commit
Verification: 82 atomic-write tests, 7 workspace-skills tests, all 276 ACP HTTP transport tests, changed-file ESLint/Prettier, repository build, and repository typecheck all passed. Two consecutive broad self-audit passes found no further actionable issue. |
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 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, chunk 29, chunk 30, chunk 31, chunk 32, chunk 33, chunk 34, chunk 35, chunk 36, chunk 37, chunk 38, chunk 39, chunk 40, chunk 41, chunk 42, chunk 43, chunk 44, Test coverage matrix (whole-diff), Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Invariant agent A: state, timers, collections — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent A: state, timers, collections — packages/cli/src/serve/workspace-registry.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/workspace-registry.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/workspace-registry.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.
— qwen3.7-max via Qwen Code /review
|
Addressed the latest review batch in commit
Verification: all 10 workspace trust reconciler tests, changed-file ESLint/Prettier, repository build, and repository typecheck passed. Two consecutive broad self-audit passes found no further actionable issue. |
wenshao
left a comment
wenshao
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 33, chunk 35, chunk 6, chunk 18, chunk 12, chunk 11, chunk 41, This PR implements hot-reload of workspace trust changes ..., chunk 25, chunk 34, chunk 4, chunk 9, chunk 42, chunk 24, chunk 5, chunk 26, chunk 37, chunk 19, chunk 10, chunk 32, chunk 44, chunk 39, chunk 22, chunk 27, chunk 21, chunk 31, chunk 14, chunk 43, chunk 20, chunk 1, chunk 7, chunk 23, chunk 28, chunk 29, chunk 17, chunk 36, chunk 38, chunk 15, chunk 13, chunk 8, chunk 2, chunk 40, chunk 16, chunk 30, chunk 3 — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. 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.8-max-preview via Qwen Code /review
|
Addressed the latest review round in
Verification: 311 targeted CLI tests passed; changed-file lint passed; full build and typecheck passed; two consecutive broad self-audit passes were clean. |
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. Unresolved, please confirm: [Critical] session.ts fork handler generation-guard cleanup — author declined with plausible architectural reasoning (fork agent runs inside parent session, no independently killable state); cannot verify without reading bridge internals Not reviewed: build-and-test — npm ci failed (ENOSPC then missing vscode-ide-companion package in worktree). Not reviewed: chunk 2 (daemon-trust-policy tests), chunk 4 (environment/settings/trustedFolders), chunk 7-23 (routes and workspace modules), chunk 26-35 (remaining serve modules), chunk 37-42 (remaining modules), chunk 46 (web-shell i18n) — territory not covered in this review pass. 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
|
Review follow-up for 82b480e\n\n| Thread | Assessment | Action |\n| --- | --- | --- |\n| Stale indexed session owner suppresses compatibility scan | Agree — correctness regression | Fixed: zero valid indexed matches now fall through to the configured live-owner scan; added a stale-index regression test. |\n| Explicit trust level depends on rule insertion order | Disagree | The full-rule effective boolean gates every single-rule preview, so a negative rule cannot be returned for an effective trusted result and vice versa; the proposed two-pass form does not change behavior. |\n| Deduplicate beginReplacement branches | Disagree | Blocked recovery intentionally supports an absent current generation after successful containment; the suggested guard would make that state unrecoverable. |\n\nVerification:\n- packages/cli workspace-registry test: 18/18 passed\n- Prettier and ESLint on both changed files: passed\n- Full npm run build: passed\n- Full npm run typecheck: passed\n- Two consecutive broad diff audits: clean\n\nThe failed review workflow on the superseded SHA was not retried because this pushed commit triggers fresh checks. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: chunk 38, chunk 43, chunk 36, chunk 3, chunk 5, chunk 23, chunk 44, chunk 39 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, chunk 1, chunk 2, chunk 4, 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 24, chunk 25, chunk 26, chunk 27, chunk 28, chunk 29, chunk 30, chunk 31, chunk 32, chunk 33, chunk 34, chunk 35, chunk 37, chunk 40, chunk 41, chunk 42, chunk 45, chunk 46, 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/cli/src/serve/run-qwen-serve.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent A: state, timers, collections — packages/cli/src/serve/workspace-registry.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/workspace-registry.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/workspace-registry.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.
— qwen3.7-max via Qwen Code /review
82b480e to
0d3db52
Compare
Document the trust hot-reload capability and reuse the daemon environment fallback so the serve process environment guard remains satisfied. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
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）为单个提交。 |
|
Resolved the merge conflicts with the latest
All 20 review threads are resolved; no existing thread required a new reply for this conflict-only update. |
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 reviewed: chunk 7, chunk 8, chunk 9, chunk 10, chunk 11, chunk 12, chunk 14, chunk 15, chunk 16, chunk 17, chunk 18, chunk 19, chunk 20, chunk 21, chunk 22, chunk 23, chunk 32, chunk 33, chunk 34, chunk 35, chunk 38, chunk 39, chunk 40, chunk 41, chunk 42, chunk 45, chunk 46 — no agent reported covering these; nobody read them. Not reviewed: chunks 2, 4, 7-12, 14-23, 25-30, 32-35, 37-42, 45-46 — not reviewed due to PR size. Not reviewed: test-matrix — not launched. Not reviewed: invariant-b — packages/cli/src/serve/run-qwen-serve.ts (counters, return values, error taxonomies). Not reviewed: invariant-c — packages/cli/src/serve/run-qwen-serve.ts (config fields, early returns). Not reviewed: invariant-b — packages/cli/src/serve/workspace-registry.ts (counters, return values, error taxonomies). Not reviewed: invariant-c — packages/cli/src/serve/workspace-registry.ts (config fields, early returns). Not reviewed: You are review agent reverse-audit — round 1. This PR (... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff. Not reviewed: chunk 44, chunk 3, chunk 5, chunk 1, chunk 13, chunk 43, chunk 31, chunk 36, chunk 24, chunk 6 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, chunk 2, chunk 4, chunk 7, chunk 8, chunk 9, chunk 10, chunk 11, chunk 12, chunk 14, chunk 15, chunk 16, chunk 17, chunk 18, chunk 19, chunk 20, chunk 21, chunk 22, chunk 23, chunk 25, chunk 26, chunk 27, chunk 28, chunk 29, chunk 30, chunk 32, chunk 33, chunk 34, chunk 35, chunk 37, chunk 38, chunk 39, chunk 40, chunk 41, chunk 42, chunk 45, chunk 46, 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/cli/src/serve/run-qwen-serve.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/run-qwen-serve.ts, Invariant agent A: state, timers, collections — packages/cli/src/serve/workspace-registry.ts, Invariant agent B: counters, return values, error taxonomies — packages/cli/src/serve/workspace-registry.ts, Invariant agent C: config fields, early returns — packages/cli/src/serve/workspace-registry.ts — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries, and cannot be certified. 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
Preserve the merge-only browser bundle allowance required by the additive workspace trust v2 SDK surface after rebasing. 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>
…nLM#7268) Drop the closed generation guard before retrying dynamic workspace runtime creation so the retried runtime starts with a fresh, open guard instead of inheriting the one closed during the abandoned attempt. Make the /workspace/reload trust reconcile fire-and-forget with a swallowed rejection (failures are reported separately), reuse sendGenerationClosedError for the memory write error path, and assert the subagent deletion commit boundary once before unlinking so a closed generation fails atomically. Add coverage for the blocked-entry deep health probe and the /session/:id/cd generation-close-during-flight path.
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>
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>
…rd, and route simplification (QwenLM#7268)
Restore the last verified trust-reconciliation and generation-guard behavior after the automated review fix marked an unconfirmed disposal as contained and removed per-scope commit checks. Defer the remaining late-round suggestions to avoid expanding the PR. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Rebased this branch onto current
Verification on new head
No review thread was newly addressed by this conflict-only rebase, so no existing thread was resolved in this cycle. |
Re-review at
|
| direction | result |
|---|---|
| main-side lines dropped by the rebase | 0 across all 15 files |
| PR-side lines dropped by the rebase | 0 (the single textual miss is getStatus(gitCwd, runtime.bridge) — correctly superseded by main's { wait } variant, not lost) |
Spot-read the merged regions where ordering matters, not just line presence:
workspace-git.ts—?wait=1preserved on both routes (:48 primary, :137 qualified), and in both cases the read is followed byassertGenerationOpen()/generationGuard?.assertOpen()beforeres.json(status). Restructuring the call into aconst statuswas necessary for that ordering; it isn't gratuitous churn.capabilities.ts/serve-features.ts— clean additions on top of main, no reordering of main's entries.serve-features.tsmemoizescachedVoiceTranscriptionAvailable, and it now depends on liveisPrimaryWorkspaceTrusted(). I chased whether a trust flip leaves it stale — it does not:runtimeActivatedcallsinvalidatePrimaryServeFeaturesCache()when the replacement runtime is primary (run-qwen-serve.ts:5385). Not a finding.
Tests on the merged tree (isolated detached worktree at 5c4ee1a0f), 1,538 passed / 0 failed:
| suite | tests |
|---|---|
run-qwen-serve + workspace-git + scheduled-tasks |
292 |
server + workspace-trust-reconciler + workspace-management |
949 |
SDK DaemonClient |
297 |
New finding — Low: the SDK browser-bundle budget has 128 bytes of headroom after the merge
packages/sdk-typescript/scripts/build.js:296 is a hard-fail gate (throw), and the merge stacks this PR's SDK additions on top of main's, which had already consumed almost all of the 170→173 KiB bump it introduced. I rebuilt the dist/daemon/index.js bundle with the verbatim esbuild config from build.js:157-172 (that bundle is built from src/, so the tsc step doesn't affect it) across four trees:
| tree | bundle bytes | budget in that tree | headroom |
|---|---|---|---|
merge base 1183a4c82 |
173,140 | 174,080 (170 KiB) | 940 |
main d61b0ea47 |
176,925 | 177,152 (173 KiB) | 227 |
PR pre-rebase 1f0f6b117 |
173,239 | 174,080 (170 KiB) | 841 |
merged 5c4ee1a0f |
177,024 | 177,152 (173 KiB) | 128 |
Attribution is exactly additive: main +3,785 B, this PR +99 B, and 176,925 + 99 = 177,024. The gate passes today — this is not a blocker and CI should stay green. But 128 bytes is 0.07% of the ceiling: any unrelated PR that lands on main first and adds even one exported type name or a slightly longer error string to the daemon SDK barrel turns this PR's build red, and the failure surfaces as an opaque Browser daemon SDK bundle is N bytes rather than anything pointing at trust hot-reload.
Related, and the reason I looked: the rebase resolved build.js by keeping main's 173 * 1024 (correct) but rewriting a historical comment line to absorb this PR's bytes:
-// Bumped from 167KB to 168KB for workspace-level streaming generation.
-// The 168KB budget also covers the daemon event-bus epoch token fields
+// Bumped from 167KB to 168KB for workspace-level streaming generation and
+// workspace trust status v2 SDK types, plus the daemon event-bus epoch tokenThe 167→168 KiB bump predates this PR, so it cannot have covered trust-status v2 types; the file's running ledger now misattributes them. Suggested fix for both points at once — restore that line and append a new one:
// Bumped from 173KB to 174KB for workspace trust status v2 SDK types
// (WorkspaceTrustStatus + reconciliation state on both daemon client classes).
const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 174 * 1024;Your call whether this clears the post-round-5 Critical-only bar — I'd argue a one-line budget bump is cheap insurance against a merge-race build break, but it is squarely a Suggestion and I won't press it.
Carried finding — Medium (perf), unchanged and still live
Re-confirmed by inspection at this head, not re-measured (the relevant files are byte-identical to the ones I measured at b222540d8): createDaemonTrustPolicyMonitor is still constructed with no pollIntervalMs (run-qwen-serve.ts:5396) → 1000 ms default (daemon-trust-policy-monitor.ts:41), and the retry ladder is still delays = [0, 50, 200] with the retryMissing gate (daemon-trust-policy.ts:78-87). daemon-trust-policy.ts, daemon-trust-policy-monitor.ts, and workspace-trust-reconciler.ts are all unchanged since my last round. Previously measured: ~254 ms per snapshot when folder trust is ON and trustedFolders.json is absent, or when user settings.json is malformed, vs ~0.6 ms healthy — a steady-state ~25% duty cycle that never self-heals. You've deferred this as "not a defect"; I still read it as a resource regression rather than a correctness bug (the pre-PR daemon polled zero), and it remains a good post-merge follow-up rather than a merge blocker.
Verdict
Merge-ready. The rebase preserved both sides everywhere I could check mechanically and by reading the merged regions; 1,538 tests green on the merged tree; CI has no failures. The two open items — 128-byte bundle headroom (Low) and the 1 Hz poll ladder (Medium, perf) — are both non-blocking.
Note on method: my local npm run build for the SDK fails with TS2307 on @qwen-code/qwen-code-core/transcriptRecords and @qwen-code/acp-bridge/transcriptReplay in src/daemon/ui/chat-record-transcript.ts. That is a symlinked-node_modules artifact of my worktree setup, not this PR — the identical four errors reproduce on unmodified main (d61b0ea47) with the same setup, and that file is not in this PR's diff. Bundle sizes above were measured with esbuild directly, bypassing tsc.
中文说明
在 5c4ee1a0f（rebase 后）的复审 —— rebase 冲突解决正确，新增 1 个 Low 级发现
相对我上一轮（1f0f6b117）的增量：rebase 到 d61b0ea47，外加 scheduled-tasks.{ts,test.ts} 中 4 行纯粹的 import/属性顺序调整（无语义变化）。第 15 轮的 containment 改动与作者的纠正性回退在磁盘上相互抵消，因此真正需要审查的只有冲突解决本身。
我上一轮提出的 blocker 已被正确解决。 我曾指出 run-qwen-serve.ts 中 main 的 channelManagementService 与本 PR 的 validateWorkspaceRuntimeForPublication 落在同一锚点、共用结尾 };——简单的 ours/theirs 解决可以编译通过，却会静默丢掉其中一侧（丢掉 PR 一侧就等于删除它自己的安全不变量 #6）。当前解决方案两侧都保留了：channelManagementService 在 :4990–5026，validateWorkspaceRuntimeForPublication 在 :5028–5067，二者都接入了同一个 createServeApp deps 对象（:5074 与 :5134）。
Rebase 校验
有 15 个文件同时被 main 的推进和本 PR 修改。我对每个文件都把 main 新增行与 PR 新增行分别与合并后的 blob 做了比对：
| 方向 | 结果 |
|---|---|
| 被 rebase 丢掉的 main 侧行 | 全部 15 个文件均为 0 |
| 被 rebase 丢掉的 PR 侧行 | 0（唯一一处文本缺失 getStatus(gitCwd, runtime.bridge) 是被 main 的 { wait } 版本正确取代，并非丢失） |
对顺序敏感的合并区域我做了实际阅读，而不只看行是否存在：
workspace-git.ts——?wait=1在两条路由上都保留（:48 主路由、:137 workspace-qualified），且两处都是先读取、再assertGenerationOpen()/generationGuard?.assertOpen()、最后才res.json(status)。把调用改写成const status正是为了这个顺序，不是无谓改动。capabilities.ts/serve-features.ts—— 在 main 之上的干净新增，未打乱 main 的条目顺序。serve-features.ts缓存了cachedVoiceTranscriptionAvailable，而它现在依赖实时的isPrimaryWorkspaceTrusted()。我追查了 trust 翻转后该缓存是否会过期——不会：runtimeActivated在替换 runtime 为 primary 时会调用invalidatePrimaryServeFeaturesCache()（run-qwen-serve.ts:5385）。不构成问题。
合并树上的测试（在 5c4ee1a0f 的隔离 detached worktree 中）：1,538 通过 / 0 失败（run-qwen-serve+workspace-git+scheduled-tasks 292；server+workspace-trust-reconciler+workspace-management 949；SDK DaemonClient 297）。
新发现 —— Low：合并后 SDK 浏览器包体积距离预算仅剩 128 字节
packages/sdk-typescript/scripts/build.js:296 是硬失败门禁（throw），而这次合并把本 PR 的 SDK 新增叠加到了 main 之上——main 引入的 170→173 KiB 提升几乎已被它自己用尽。我用 build.js:157-172 中原封不动的 esbuild 配置重建了 dist/daemon/index.js（该包从 src/ 打包，因此 tsc 步骤不影响它），覆盖四棵树：
| 树 | 包体积（字节） | 该树中的预算 | 余量 |
|---|---|---|---|
合并基点 1183a4c82 |
173,140 | 174,080（170 KiB） | 940 |
main d61b0ea47 |
176,925 | 177,152（173 KiB） | 227 |
PR rebase 前 1f0f6b117 |
173,239 | 174,080（170 KiB） | 841 |
合并后 5c4ee1a0f |
177,024 | 177,152（173 KiB） | 128 |
归因完全可加：main +3,785 B，本 PR +99 B，且 176,925 + 99 = 177,024。门禁当前是通过的——这不是 blocker，CI 应当保持绿色。但 128 字节只占上限的 0.07%：任何先于本 PR 合入 main、并向 daemon SDK barrel 增加哪怕一个导出类型名或稍长错误字符串的无关 PR，都会让本 PR 构建变红，而报错会表现为难以定位的 Browser daemon SDK bundle is N bytes，完全指向不到 trust hot-reload。
与此相关（也是我去查这个的原因）：rebase 在解决 build.js 时保留了 main 的 173 * 1024（正确），却改写了一行历史注释来吸收本 PR 的字节数。167→168 KiB 那次提升早于本 PR，不可能覆盖 trust-status v2 类型，该文件的账目因此产生了错误归属。建议一次性修正两点：还原那一行，并追加新的一行，同时把预算提到 174 * 1024。
是否符合第 5 轮后「仅 Critical」的门槛由你判断——我认为一行预算提升是应对合并竞态导致构建中断的廉价保险，但它确实属于 Suggestion 级别，我不会坚持。
延续的发现 —— Medium（性能），未变且仍然存在
在此 head 上通过阅读代码确认（未重新测量，因为相关文件与我在 b222540d8 测量时逐字节相同）：createDaemonTrustPolicyMonitor 仍未传 pollIntervalMs（run-qwen-serve.ts:5396）→ 默认 1000 ms（daemon-trust-policy-monitor.ts:41），重试阶梯仍为 delays = [0, 50, 200] 且保留 retryMissing 门控（daemon-trust-policy.ts:78-87）。daemon-trust-policy.ts、daemon-trust-policy-monitor.ts、workspace-trust-reconciler.ts 自上一轮以来均未改动。此前测得：folder trust 开启且 trustedFolders.json 缺失时、或用户 settings.json 格式错误时，每次快照约 254 ms，而健康路径约 0.6 ms——这是约 25% 占空比的稳态开销，且不会自愈。你已将其判定为「非缺陷」并推迟；我仍认为它是资源层面的回退而非正确性缺陷（PR 之前的 daemon 完全不轮询），适合作为合并后的跟进项，而不是合并阻塞项。
结论
可以合并。 无论是机械比对还是逐段阅读合并区域，rebase 都保留了两侧改动；合并树上 1,538 个测试通过；CI 无失败项。两个未决项——128 字节的包体积余量（Low）与 1 Hz 轮询阶梯（Medium，性能）——均不阻塞合并。
方法说明：我本地对 SDK 执行 npm run build 会在 src/daemon/ui/chat-record-transcript.ts 上报 TS2307（找不到 @qwen-code/qwen-code-core/transcriptRecords 与 @qwen-code/acp-bridge/transcriptReplay）。这是我 worktree 中 node_modules 软链接造成的环境问题，与本 PR 无关——在相同配置下、未修改的 main（d61b0ea47）上会复现完全一致的四个错误，且该文件不在本 PR 的 diff 中。上表包体积是直接用 esbuild 测得、绕过了 tsc。
|
@wenshao Thanks for the careful post-rebase verification. I independently checked the exact I am deferring both the 174 KiB budget/comment cleanup and the carried polling-performance item. This PR is well past the five-round threshold, and neither item is a current correctness, security, data-loss, or regression blocker: the bundle gate passes now, CI has no failure, and the performance item is explicitly non-blocking. Raising the budget preemptively would widen this review round and weaken the existing size guard; if No code change in this round. |
|
@qwen-code /triage |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action takenThe post-rebase re-review at
Both items are non-blocking Suggestions/observations, not correctness, security, data-loss, or regression defects. Per the post-5-round policy, only Critical fixes are landed at this stage. No code change is warranted. 中文说明未做任何改动针对
两项均为非阻塞的建议/观察，不属于正确性、安全性、数据丢失或回归缺陷。按照五轮后仅处理 Critical 的策略，本阶段不做代码改动。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Triage re-run completed without a new review. The stage comments above were updated with the latest result. View workflow run. |
| color: params['color'] as SessionGroupColor, | ||
| }); | ||
| assertGenerationOpen?.(); | ||
| this.replyConn(conn, id, { group }); |
There was a problem hiding this comment.
[P2] Session group mutations (create/update/delete) commit to disk before assertGenerationOpen?.() is checked, but unlike scheduled tasks there is no rollback path. If the generation closes between the mutation and the assertion, the client receives a 503 workspace_runtime_unavailable error while the session group change is already persisted.
This is likely acceptable since:
- Session groups are lightweight metadata
- The next generation reconciles from disk state
- The race window is tiny
But it would be worth a brief comment here explaining why rollback is intentionally omitted (vs. the rollbackCronMutation pattern used for scheduled tasks), so future readers don't wonder if it's a gap.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Incremental Review (10 commits since last approval)
Reviewed the 654-line incremental diff covering generation guard hardening, containment retry preservation, and session group mutation guards.
Summary
All 10 fix commits are correct and well-targeted. The changes close real race conditions without introducing regressions.
Generation gap fixes — properly close the races:
assertGenerationOpen?.()after session group create/update/delete catches generation closure between mutation and reply- Branch session cleanup now removes persisted session records when
killSessionsucceeds, preventing orphan metadata - Settings commits (
persistDisabledTools/persistDisabledSkills) thread the generation guard through tosetValue
Containment retry semantics — correctly preserved:
- The
rollbackBefore && rollbackAfterguard in scheduled tasks ensures generation-closed errors are only thrown when a mutation was actually committed, so legitimate 404/409 responses (task_not_found, max_tasks_reached, task_legacy_unsupported) are returned to the client unchanged - The overCap path correctly returns
max_tasks_reachedinstead of a misleadingworkspace_runtime_unavailable
Test coverage — thorough and discriminating:
- 8 new test cases covering all the fix paths
- The persistent-ENOENT test for trust policy correctly distinguishes transient vs persistent file disappearance
- The concurrent max-tasks test validates the exact
checkscount to ensure the generation guard fires the right number of times - The branch cleanup test parametrizes over kill success/failure to verify
removeSessionis conditional
One P2 comment
Left one inline comment on dispatch.ts: session group mutations lack a rollback path (unlike scheduled tasks). Acceptable given the small race window and next-generation reconciliation, but worth a brief code comment explaining the intentional asymmetry.
Verdict
No P0/P1 issues. The incremental changes strengthen the PR's correctness guarantees. LGTM.
|
Released in v0.21.1. |
6 participants
What this PR does
This PR makes workspace trust changes take effect in a running daemon without requiring a process restart. It introduces semantic trust-policy snapshots and monitoring, reconciles each affected workspace into a new runtime generation, and closes the previous generation before draining its sessions and services.
All workspace-scoped daemon surfaces now resolve the selected workspace generation and fail closed while trust reconciliation is applying, failed, blocked, draining, or otherwise unavailable. Trust grants rebuild settings, environment, filesystem, tools, services, and caches from fresh state; trust revocations prevent further work on the old trusted generation before asynchronous cleanup begins.
The additive v2 trust API reports configured trust, effective trust, policy revision, and reconciliation state. The TypeScript SDK exposes that status, while the existing v1 request-only API remains compatible. Web Shell integration is intentionally excluded from this PR and can be handled separately.
Why it's needed
Workspace trust was previously evaluated when a runtime started. Changing user, system, default-folder, IDE, or trusted-folder policy while the daemon was running could leave a stale trusted runtime, sessions, services, environment, filesystem view, and settings caches active until restart. That is both confusing to users and unsafe on trust revocation. Runtime-generation reconciliation makes the effective state observable and applies policy changes without restarting the daemon.
Reviewer Test Plan
How to verify
applyingtostable; and the TypeScript SDK reports the same configured and effective trust state.Evidence (Before & After)
Before: changing trust policy only submitted a request; the running workspace could keep its boot-time runtime and cached capabilities until daemon restart, and clients could not distinguish configured trust from effective trust or an in-progress reconciliation.
After: policy changes close and replace the affected runtime generation in process, workspace routes fail closed during the transition, and the v2 API plus TypeScript SDK expose applying, failed, blocked, configured, and effective state. This PR has no Web Shell UI change, so screenshots or recordings are N/A.
Tested on
Environment (optional)
macOS, Node.js v22.22.3, npm workspace build and Vitest, sandbox not used.
Risk & Scope
Linked Issues
Part of #6378
中文说明
本 PR 做了什么
本 PR 让 workspace trust 变更能够在正在运行的 daemon 中生效，无需重启进程。它引入语义化的 trust policy 快照与监控，为每个受影响 workspace 协调新的 runtime generation，并在 drain 其中的 session 和 service 之前先关闭旧 generation。
所有 workspace scoped daemon 能力现在都会解析选定 workspace generation，并在 trust reconciliation 处于 applying、failed、blocked、draining 或其他不可用状态时 fail closed。授予 trust 时会从最新状态重新构建 settings、environment、filesystem、tools、services 和 caches；撤销 trust 时则会在异步清理开始前阻止旧 trusted generation 继续接收工作。
新增的 v2 trust API 会报告 configured trust、effective trust、policy revision 和 reconciliation state。TypeScript SDK 暴露该状态，现有 v1 request-only API 保持兼容。Web Shell 集成被有意排除在本 PR 之外，可在后续单独处理。
为什么需要它
此前 workspace trust 只在 runtime 启动时评估。daemon 运行期间修改 user、system、default-folder、IDE 或 trusted-folder policy，可能导致过期的 trusted runtime、sessions、services、environment、filesystem view 和 settings caches 一直保留到重启。这既会让用户困惑，也会在撤销 trust 时产生安全风险。runtime-generation reconciliation 让 effective state 可观测，并能在不重启 daemon 的情况下应用 policy 变更。
Reviewer 测试计划
如何验证
applying进入stable；TypeScript SDK 报告相同的 configured 与 effective trust state。证据（Before & After）
Before：修改 trust policy 只会提交 request；运行中的 workspace 可能继续保留启动时的 runtime 与 cached capabilities，直到 daemon 重启，client 也无法区分 configured trust、effective trust 或正在进行的 reconciliation。
After：policy 变更会在进程内关闭并替换受影响的 runtime generation，workspace route 在过渡期间 fail closed，v2 API 与 TypeScript SDK 会暴露 applying、failed、blocked、configured 和 effective state。本 PR 没有 Web Shell UI 变更，因此截图或录屏为 N/A。
测试平台
环境（可选）
macOS、Node.js v22.22.3、npm workspace build 与 Vitest，未使用 sandbox。
风险与范围
关联 Issue
Part of #6378