refactor: deepen architecture ownership boundaries - #9682
Conversation
508a4c6 to
8469565
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）为单个提交。 |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
# Conflicts: # packages/cli/src/serve/acp-http/dispatch.ts # packages/cli/src/serve/acp-session-bridge.ts # packages/cli/src/serve/server.ts
CI (ubuntu-latest, Node 22.x) hit the 15s test timeout on 'times out a legacy update check while its cache refresh stalls' (run 32500060626). Install fake timers only after the harness is up so app setup's real async work never runs under a frozen clock, and give the test a 30s budget for the shared-runner contention this package's vitest config already documents.
… baseline QwenLM#9587 (b5fbdb2) grew cd-cua-driver.yml from 29715 to 42519 bytes on main without updating the ratchet, so every PR that merges main now fails the workflow-size check. Record the shipped size, following the same fix as QwenLM#9747 for qwen-autofix.yml.
The workflow grew from 29715 to 42519 bytes when QwenLM#9587 landed on main without a baseline update (a latent main-side violation every PR merging main now trips, as the check message itself directs). Record the new size following the same remedy as QwenLM#9747/QwenLM#9682.
Same latent main-side violation as QwenLM#9682/QwenLM#9683-era fixes: QwenLM#9587 grew the workflow without a baseline update; record the new size as the check message directs (precedent QwenLM#9747).
Same latent main-side violation as fixed in QwenLM#9682: QwenLM#9587 grew the workflow without a baseline update; record the new size as the check message directs (precedent QwenLM#9747).
* refactor(core): shrink content generator interface * refactor(core): remove orphaned request-tokenizer estimator cluster Removing countTokens from both providers deleted the last production consumers of RequestTokenEstimator. Delete the orphaned cluster: requestTokenizer.ts (330), imageTokenizer.ts (534), types.ts (36), the directory barrel (11), and both test files (608 lines). Also drop the inert vi.mock of requestTokenizer.js left in client.test.ts and the stale dimension-extractor cross-reference in review/lib/assets.ts. textTokenizer.ts and supportedImageFormats.ts stay: converter.ts, pdf.ts, and fileUtils.ts still consume them and the core barrel re-exports them. * docs(design): sync lazy-google-genai-loading record with shrunk interface countTokens and useSummarizedThinking no longer exist on ContentGenerator, so the design record for the lazy-wrapper architecture must not keep advertising them: list the three remaining shared async operations, drop the useSummarizedThinking sentence and the summarized-thinking item from the consumer audit and Verification section, and add a dated note recording the interface shrink from PR QwenLM#9676. * ci: record cd-cua-driver.yml size growth in .size-baseline Same latent main-side violation as fixed in QwenLM#9682: QwenLM#9587 grew the workflow without a baseline update; record the new size as the check message directs (precedent QwenLM#9747). * docs: finish scrubbing tokenizer references after estimator-cluster removal Follow-up to 0ee1763/1871bb5b81 (review round 2): - supportedImageFormats.ts header and getSupportedImageFormatsString doc no longer describe a tokenizer decode/metadata-extraction stage; the list is now documented as the vision-input acceptance list, with token accounting noted as the flat DEFAULT_IMAGE_TOKEN_ESTIMATE. - web-shell-image-drag-and-drop.md's BMP rationale no longer claims ImageTokenizer parses BMP dimensions; dated sync note added stating BMP support rests on SUPPORTED_IMAGE_MIME_TYPES plus converter passthrough since PR QwenLM#9676. * docs: drop tokenizer from the BMP test-plan line Follow-up to 18f08c0: the test plan still required converter/tokenizer focused tests for image paths; the image-tokenizer estimator cluster was removed in PR QwenLM#9676 (text tokenizer is unaffected and out of scope here).
# Conflicts: # packages/web-shell/client/App.tsx
|
Re-run at the new head ( Template looks good ✓ — all sections present, bilingual body included. Problem: unchanged from the prior pass and still concrete, not theoretical — four near-identical trusted-runtime resolver clones across the workspace routes, ACP transport budgets split between two modules, a 159-line CLI re-export shim in front of the owning package, and duplicated admission/retry boolean pairs in Web Shell. Since the last review the author also landed the round-1 fixes (the estimator regression repair, exemption-release pinning, and the R9-1 load-identity guard), which this pass re-gated. Direction: aligned. Consolidating the daemon's safety seams (bounded transport, workspace trust selection, prompt admission, attachment epochs) keeps those invariants auditable in one place each, and the committed design doc states the behavioral invariants up front. The follow-up stages from the same design doc have since been landing as separate PRs, which is the staging this one argued for. Size: core infrastructure (cross-package: Approach: each seam still reads as a faithful move plus dedup rather than a rewrite — the new modules expose decisions and transitions (budgets, trust selection, attempt classification, load epochs), not React setters or Express responses. No drive-by changes spotted at gate level. Risk: no matches against the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明在新 head（ 模板完整 ✓ —— 各部分齐全，含中文正文。 问题： 与上一轮相同，仍是具体存在的重复而非理论问题——workspace 路由中四份几乎相同的 trusted-runtime resolver 克隆、ACP 传输预算分散在两个模块、CLI 侧 159 行的 re-export shim 挡在所属 package 前面、Web Shell 中重复的 admission/retry 布尔对。自上次审查以来，作者还合入了第一轮修复（估算器回归修复、豁免释放钉测、R9-1 load 身份守卫），本轮对其重新做了门控检查。 方向： 对齐。集中管理 daemon 的安全接缝（有界传输、workspace trust 选择、prompt admission、attachment epoch）使这些不变量可在各自唯一位置被审计，且随 PR 提交的设计文档预先声明了行为不变量。同一设计文档的后续阶段此后已按独立 PR 陆续合入，正是本 PR 主张的分阶段方式。 规模： 核心基础设施（跨 4 个 package： 方案： 每个接缝仍是忠实的搬移加去重，而非重写——新模块暴露的是决策与状态迁移（预算、trust 选择、尝试分类、load epoch），而不是 React setter 或 Express response。门控层面未发现顺手改动。 风险： 未命中与 revert 相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (re-run at
|
| File | What changed |
|---|---|
packages/acp-bridge/src/transport-safety.ts |
New module owning budgets, byte estimation (with the hazardous-toJSON repair), log-safe client, guarded connection proxy |
packages/acp-bridge/src/bridge.ts |
Safety code lifted out; binds the new module at the two adapter points |
packages/acp-bridge/src/spawnChannel.ts |
Budget and teardown wiring collapsed into createBoundedAcpTransportSafety |
packages/acp-bridge/src/transport-safety.test.ts |
New: estimator byte-parity and refusal cases, admission release pairing, null-normalization |
packages/cli/src/serve/acp-session-bridge.ts |
Deleted: the 159-line CLI-local re-export shim |
packages/cli/src/serve/workspace-route-runtime.ts |
resolveTrustedRuntime added as the single trusted-selection policy |
packages/cli/src/serve/index.ts |
Public serve re-exports preserved, sourced from owning package subpaths |
packages/cli/src/serve/fast-path.test.ts |
Import-boundary assertions updated for the shim deletion |
packages/web-shell/client/prompt-admission.ts |
New: attempt classifier plus transcript identity helpers moved verbatim |
packages/web-shell/client/App.tsx |
Four send/retry sites adapted to PromptAdmissionAttempt |
packages/web-shell/client/prompt-admission.test.ts |
New: classifier truth table, identity matching, stale-owner cases |
packages/webui/src/daemon/session/attachment-lifecycle.ts |
New: pending-load epochs, manual-clear flag, load-tied cleanup-detach exemption |
packages/webui/src/daemon/session/DaemonSessionProvider.tsx |
Scattered refs replaced by one lifecycle instance; R9-1 adoption guard |
docs/design/architecture-deepening-2026-08-21.md |
Design doc: invariants, module direction, staging, non-goals |
| …and 76 more files | Route migrations, test re-homes, type-only import moves, developer-doc updates |
Test evidence (PR's own CI at 5be8b5dd — no PR code executed in this run)
Every substantive check on this head is green, including Test (ubuntu-latest) — the release-note-classification failure the author flagged on the earlier head (inherited from main at the time) is no longer red here. The macOS/Windows unit-matrix and CLI-sandbox integration checks are skipped by workflow design on this event, same as prior heads. Standout signals: Serve A/B re-ran base-vs-head on this head with zero response differences, Real daemon E2E and web-shell E2E smoke both pass, and the Security Checks bundle (CVE audit, TruffleHog) is clean.
Two notes on attribution. (1) The web-shell visuals preview still reports scenario render failures on this head — this PR touches zero files under client/e2e/ (verified against the file list), and the maintainer reproduced the same two failures identically on the merge-base arm, pinning them to a pre-existing selector-strictness bug in the visuals spec on main; the capture job itself is green. (2) After the bot's deferral, maintainer @wenshao ran an independent local A/B at this exact head (200k-value estimator oracle, 489 live route probes, compiler-API export enumeration, exhaustive admission truth table) and posted his seam-by-seam results in this thread — that is his verification evidence, converging with this static review, and he has approved.
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Serve A/B (ubuntu-latest, Node 22.x) | success |
| Real daemon E2E / Java 11 | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| Capture web-shell visuals (ubuntu-latest, Node 22.x) | success |
| Desktop Shell (windows-2022) | success |
| Desktop Shell (ubuntu-22.04) | success |
| Integration Tests (no-AK, No Sandbox) | success |
| precheck-pr / precheck | success |
| Dependency CVE audit | success |
| Secret scan (TruffleHog) | success |
| Classify PR / label | success |
Sandboxed verification is already in flight: the verify job of this very triage run is executing now and will post its A/B report here on completion — no fresh trigger needed. tmux real-scenario run: N/A — unattended CI path, and this is a behavior-preserving internal refactor with nothing user-visible to drive.
中文说明
代码审查（在 5be8b5dd 上重跑）
完整重读了 diff，包括上轮之后新增的全部内容：R1-1 估算器修复、豁免释放钉测、Web Shell restore 块移植、R9-1 load 身份守卫，以及多次对 main 的 rebase。未发现阻塞项——各接缝仍是忠实的搬移加去重，且修复均正确。
接缝 1 —— 传输安全。 我直接对照被删除的旧代码验证了单调性论断：estimateTransportValueBytes 就是旧出站估算器把 Object.hasOwn(v, 'toJSON') 换成 hasStructuralToJSONHazard(v)，而危险判定（自有 toJSON 可调用或为 accessor）是旧拒绝条件的真子集。拒绝集只会缩小，字节算术其余部分完全一致——因此出站放宽只可能放行纯数据 toJSON 值，不可能误估大小，入站对 wire 合法（JSON.parse 产物）参数的准入也得以保持。这正是第一轮回归修复的形状，且新的 transport-safety.test.ts 以杀变异方式钉住：数据型 toJSON 形态与 JSON.stringify 字节对齐、可调用/accessor toJSON 仍被拒绝（含危险检查是唯一防线的数组场景）、循环拒绝且遍历有界、以及准入层的释放配对与 undefined→null 预留归一化测试。spawnChannel.ts/bridge.ts 中的一次性退休、消息观测释放与 guard 映射均未改变。
接缝 2 —— 可信 workspace 路由。 逐一审计了全部迁移：每处 resolveWorkspaceRuntimeFromParam → resolveTrustedRuntime 替换都同时删除了同位置的 requireTrustedWorkspaceRuntime 检查，因此没有路由新增 403，也没有路由失去 trust 门；保留未信任解析器的两个文件对应有意放宽的只读路径。响应码（workspace_mismatch、workspace_runtime_unavailable、untrusted_workspace）未变。
接缝 3 —— CLI 兼容层删除。 约 30 个导入方机械地改指向所属 @qwen-code/acp-bridge 子路径；serve 公共 barrel 保持相同导出集（工厂、sendBridgeError 按 instanceof 匹配的错误类子集、类型别名），改为从所属模块取源，并恢复了说明其公共面作用的注释。fast-path 导入边界测试随 shim 删除做了对应更新，而非削弱。
接缝 4 —— prompt admission。 抽出的 8 个 helper 是忠实搬移，classifyFailure 真值表在全部四处调用点精确复现旧 admissionStarted/admitted 布尔对（'unknown' ⇔ 已开始且未准入且非明确拒绝；'after-admission' ⇔ 已准入）。所有权在每个守卫处实时求值，切换 session 后 retry 仍无法提交。
接缝 5 —— attachment 生命周期。 所有结算/拒绝/释放位置都有 load 身份与 generation 守卫：被取代的 load 无法结算当前 attachment；watchdog 在调用 onTimeout 之前释放；R9-1 改动范围正确——迭代局部的 attemptedLoadForIteration 阻止被取代的迭代认领后继者的 pending load（保留了创建流程所依赖的、有注释说明的无 load 捕获旁路），且 releaseCleanupDetachExemption 现在要求保留方 load 的身份，被取代 switch 的迟到释放不再能清掉后继者的豁免。我上一轮的两个判断点均已落定：估算器差异被确认为修复（维护者下述逐接缝 A/B 独立发现出站放宽阻止了 main 上一个真实的子进程误杀），无条件清除形态在生产中不可达（唯一调用点两个参数都传了实值），而新守卫比 main 更严格。
测试证据（5be8b5dd 上 PR 自身 CI——本次运行未执行任何 PR 代码）
本 head 上所有实质性检查均为绿色，包括 Test (ubuntu-latest)——作者在较早 head 上指出的 release-note 分类失败（当时继承自主干）在此已不再为红。macOS/Windows 单测矩阵与 CLI 沙箱集成检查按工作流设计在此事件下为 skipped，与之前的 head 一致。突出信号：Serve A/B 在本 head 上重跑 base 与 head 对比，响应零差异；真实 daemon E2E 与 web-shell E2E smoke 均通过；安全检查组合（CVE 审计、TruffleHog）干净。
两点归属说明。（1）web-shell 视觉预览在本 head 上仍报告场景渲染失败——本 PR 未触碰 client/e2e/ 下任何文件（已对照文件列表核实），且维护者在 merge-base 侧复现出完全相同的两处失败，定位为干线视觉用例中既有的选择器严格性 bug；截图任务本身为绿。（2）在机器人缓议之后，维护者 @wenshao 在与本 head 完全一致的提交上独立做了本地 A/B（20 万值估算器 oracle、489 次实况路由探测、编译器 API 导出面枚举、穷举准入真值表），并在本线程发布了逐接缝结果——那是他的验证证据，与本静态审查相互印证，他已批准。
沙箱验证已在路上：本次 triage 运行的 verify 任务正在执行，完成后会自动在此发布 A/B 报告——无需再次触发。tmux 真实场景：不适用——无人值守 CI 路径，且本 PR 是行为保持的内部重构，没有可操作的用户可见变化。
— Qwen Code · qwen3.8-max
Reviewed at 5be8b5dd43da950851135c3f5eb7e26bc06207d1 · re-run with @qwen-code /triage
|
Confidence: 3/5 — clean review, and this head now carries green CI plus a maintainer's seam-by-seam verification; the cap is pure policy — the fork-refactor guardrail keeps the final approval human. Stepping back: my two reservations from the first pass are both resolved, and resolved in the interesting direction. The estimator swap I flagged as "a policy change embedded in the move" turned out to be the most valuable part of the PR — the round-1 fix restored inbound parity, and the maintainer's A/B then showed the outbound relaxation is monotone and stops a real child-kill on Everything else converges: the static review at this head finds the seams faithful, the trust gates preserved one-for-one, and the public CLI surface intact; the PR's own CI is fully green on Per policy I'm still not approving: a cross-repository 中文说明置信度：3/5 —— 审查干净，且本 head 已有全绿 CI 与维护者逐接缝验证；封顶纯属策略——fork 重构护栏规定最终批准必须由人类做出。 退一步看：我第一轮的两个保留意见都已落定，而且落定的方向很有意思。我曾把估算器替换标记为「搬移中夹带的策略变化」，结果它成了本 PR 最有价值的部分——第一轮修复恢复了入站等价性，维护者的 A/B 随后证明出站放宽是单调的，并且止住了 其余各方相互印证：本 head 的静态审查显示各接缝忠实、trust 门一一对应保留、CLI 公共面完整；PR 自身 CI 在 按策略我仍不做批准：跨仓库（fork）的 — Qwen Code · qwen3.8-max Reviewed at |
|
⏸️ Deferring to @wenshao — re-run at 中文说明⏸️ 转交 @wenshao —— 在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…e estimation The estimator unification in 8469565 silently swapped inbound handler admission from the deleted lenient estimateAcpHandlerBytes to the strict outbound estimator, which refused any object or array carrying an own toJSON key regardless of its value. Inbound params are JSON.parse'd wire data, where toJSON is plain data that JSON.stringify serializes as an ordinary member, so the refusal retired whole daemon transports over wire-legal messages (guard.fail destroys the child's stdio and terminates the child in the spawnChannel wiring). Restrict the refusal to the shapes that actually defeat structural estimation: callable own toJSON (the JSON.stringify substitution hazard) and accessor-backed toJSON. Plain-data toJSON keys are estimated like any other member. Pinned by transport-safety.test.ts (byte parity with JSON.stringify, continued callable/accessor/cyclic refusal, and admission-level coverage); the tests fail against the blanket Object.hasOwn check.
rejectPendingLoad releases the cleanup-detach exemption for the failed load's session, and cancelPendingLoad routes through it; nothing in the existing suite exercised that coupling (deleting the releaseCleanupDetachExemptionForSession call kept all lifecycle, actions, and provider tests green). Add reject/cancel cases asserting the promise settles and the exemption is gone afterwards, plus a scope case keeping another session's exemption intact. The first two fail against the deletion mutant.
…el rationale workspace-agents.ts re-exported InvalidClientIdError for test files, but the same refactor re-pointed the only such test (workspace-memory.test.ts) to import from @qwen-code/acp-bridge/bridgeErrors directly, leaving the re-export with zero consumers and the file never uses the symbol internally. Delete the re-export, its now-false rationale comment, and the unused import; workspace-agents.ts is not on the CLI's public barrel, so no external surface changes. Conversely, restore a shortened rationale above the serve barrel's typed error re-export block: sendBridgeError still matches every class there via instanceof, and with all in-repo callers now importing bridgeErrors directly the comment is the only surviving argument against a future 'remove unused exports' cleanup that would break external embeds.
Round-1 review response — all 5 threads verified against code, all validEach finding was checked against the code at R1-1 (Critical, both locations) — confirmed: real regression, fixedThe behavior-preserving claim was broken on the inbound path. Evidence trail:
Fix (both sites, array + record branches): Pinned by the new R1-3 (Suggestion) — confirmed: gap was real, tests addedVerified: the lifecycle suite covered supersede, stale-settle, watchdog, and direct preserve/release, but never reject/cancel with an exemption held — deleting the R1-4 (Suggestion) — confirmed: rationale restoredVerified: R1-5 (Suggestion) — confirmed: dead surface removedVerified: the only references to Additional verification (open item from the triage deferral): web-shell visuals render failureNot caused by this PR. The visuals workflow (run 32737065411) renders both the PR base and this head in one job: the |
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: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/webui/src/daemon/session/attachment-lifecycle.test.ts:78 — [probe] timeout test asserts only post-conditions; release-before-callback ordering unpinned — deferred under the round-2 code-age rule (anchored on code unchanged since th…
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
… arms; fix barrel comment
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: reverse audit — did not converge within the reverse-audit round cap of 10.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/transport-safety.test.ts:151 — [probe] record-cycle test pins only the refusal sentinel — the record-side seen-check mutant survives (array leg is pinned)packages/webui/src/daemon/session/attachment-lifecycle.test.ts:60 — [probe] timeout test asserts only post-conditions; release-before-callback ordering unpinnedpackages/webui/src/daemon/session/attachment-lifecycle.test.ts:94 — [probe] releaseCleanupDetachExemption identity guard never exercised with a non-matching session
Convergence: round 3 posted 5 inline comment(s), 4 of them reported for the first time; the previous round posted 9 (9 new). Findings keep coming back to the same files: packages/acp-bridge/src/transport-safety.test.ts (findings in round 2; 2 more now); packages/webui/src/daemon/session/attachment-lifecycle.test.ts (findings in round 2; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 10。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 5 条行内评论，其中 4 条是首次提出；上一轮发布了 9 条（其中 9 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/transport-safety.test.ts（第 2 轮已出过发现，本轮又有 2 条）；packages/webui/src/daemon/session/attachment-lifecycle.test.ts（第 2 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
…dler-count/null-normalization/supersede-release
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent invariant-c (packages/acp-bridge/src/spawnChannel.ts)": none — no check was cut short..
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/transport-safety.ts:223 — [probe] no test pins the observeMessage direction === 'sent' guard — the release-on-received mutant survives the whole acp-bridge suite
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent invariant-c (packages/acp-bridge/src/spawnChannel.ts)"：none — no check was cut short.。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
CI note: the only failing check is 中文说明：唯一失败的检查是 main 自身已连续三天在同一子测试上失败的 classify-release-notes 辅助测试；本分支未改动该脚本与测试（与 main 逐字节一致），红色继承自主干，其余检查全绿。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Not explored to full depth (tool budget reached): chunk 4: executed the test suite in a throwaway tree ( npm ci not feasible in this worktree within budget); substituted exhaustive hand-tracing of all nine tests agains….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/App.tsx:6685 — [review] classifyFailure's admitted-over-definitelyRejected precedence — load-bearing for this branch's equivalence — is pinned by no testpackages/acp-bridge/src/transport-safety.test.ts:44 — [review] estimator number arm and undefined half are exercised by no test — two invisible mutants (probe-verified)packages/acp-bridge/src/transport-safety.ts:235 — [review] any pipeLimits with maxQueuedBytes <= 2048 passes validation but kills the channel on the first guarded operation (probe-verified)packages/acp-bridge/src/transport-safety.ts:590 — [review] releaseTerminal/killTerminal ?? {} normalization — load-bearing for reserve/release identity pairing — is pinned by no test (mutant-probed)packages/webui/src/daemon/session/attachment-lifecycle.test.ts:316 — [review] loadless-release-vs-load-associated-exemption shape — the exact call shape all three production release sites use — is pinned by no test (mutant-probed)packages/acp-bridge/src/transport-safety.ts:64 — [review] error-frame reserve→release pairing (releaseMessage's message['error'] branch) works today but is pinned by no test (probe-verified)packages/acp-bridge/src/transport-safety.test.ts:254 — [review] admission budget release on the handler-exception path (the finally decrements) is pinned by no test (mutant-probed)
Convergence: round 7 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push keeps the loop from re-deriving the same set; this PR's reviews already resolve to a critical posting floor. (Observation only — nothing was withheld from this review because of this observation.)
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
Residual risk: this loop is persistently critical — Criticals stood in the previous round's work-list and stand again this round (1 Critical(s)), the rate of first-time findings is not falling (this round 1, previous 1), and the standing Critical backlog is not shrinking. The severity floor will not converge it. Recommendation: land-with-residual-risk — the exit is a maintainer risk-acceptance decision (merge, carrying the residual risk), not another review round. Residual-risk inventory for that decision (maintainer to complete):
| standing Critical | attack surface | attacker-dependency | blast radius |
|---|---|---|---|
| (each standing Critical) | … | … | … |
Advisory only — it does not block this review.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 3。
未探索到全部深度（达到工具调用预算）：chunk 4：executed the test suite in a throwaway tree ( npm ci not feasible in this worktree within budget); substituted exhaustive hand-tracing of all nine tests agains…。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 7 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，可以避免循环反复推导同一组发现；本 PR 的评审已解析为 critical 发布下限。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical 本轮依然存在（本轮 1 条 Critical），首次发现的速率没有下降（本轮 1，上一轮 1），且未决 Critical 积压没有减少。severity floor 无法使其收敛。建议：land-with-residual-risk——出口是 maintainer 的风险接受决定（合入并承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。仅为建议——不阻断本次评审。
— qwen3.8-max via Qwen Code /review (v0.22.2)
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.
7 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R8-2 watchdog test release-before-callback ordering unpinned (attachment-lifecycle.test.ts:97) — already recorded as round-2/3/5/6/7 convergence-posture deferrals (reviews 5013080184, 5014440443, 5029145840, 5032096362, 5045295751)
- R8-3 watchdog onTimeout swallow / load-always-settles guarantee unpinned (attachment-lifecycle.ts:72) — already recorded as the round-6 convergence-posture deferral (review 5032096362)
- R8-8 estimator number arm and undefined half unpinned in acceptance direction (transport-safety.test.ts:50) — already recorded as the round-7 convergence-posture deferral (review 5045295751)
- R8-9 admission budget release on the handler-exception path unpinned (transport-safety.test.ts:251) — already recorded as round-6/7 convergence-posture deferrals (reviews 5032096362, 5045295751)
- R8-10 watchdog-timeout settle path never releases the cleanup-detach exemption, unpinned (attachment-lifecycle.ts:68) — already recorded as round-5/6 convergence-posture deferrals (reviews 5029145840, 5032096362)
- R8-12 record-cycle test pins only the refusal sentinel — record-side seen-check mutant survives (transport-safety.test.ts:152) — already recorded as the round-3 convergence-posture deferral (review 5014440443)
- R8-13 error-frame reserve-release pairing (release half + identity) unpinned (transport-safety.ts:64) — already recorded as round-5/7 convergence-posture deferrals (reviews 5029145840, 5045295751)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent invariant-a (packages/acp-bridge/src/spawnChannel.ts)": (none — no check was cut short); chunk 5: none (suite execution was skipped for the reason stated above, which is an environment constraint, not a check the ceiling stopped — every test was verified by ….
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/webui/src/daemon/session/attachment-lifecycle.test.ts:116 — [probe] watchdog clearTimeout on early settle unpinned by any testpackages/webui/src/daemon/session/attachment-lifecycle.test.ts:133 — [probe] session-identity clause of releaseCleanupDetachExemption unpinned in either direction
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 7 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — did not converge within the reverse-audit round cap of 3。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent invariant-a (packages/acp-bridge/src/spawnChannel.ts)"：(none — no check was cut short)；chunk 5：none (suite execution was skipped for the reason stated above, which is an environment constraint, not a check the ceiling stopped — every test was verified by …。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
…overnance # Conflicts: # packages/cli/src/serve/routes/session-pr-backfill.test.ts
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.
5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- fail-once transport retirement invariant unpinned (transport-safety.ts:190) — already recorded as the round-6 convergence-posture deferral (review 5032096362)
- admission count-exhaustion branch (maxActiveHandlers term) never exercised (transport-safety.test.ts) — already recorded as the round-5 convergence-posture deferral (review 5029145840)
- admission budget release on the handler-exception path / error-frame reserve-release positive control (transport-safety.test.ts) — already recorded as round-5/7 convergence-posture deferrals (reviews 5029145840, 5045295751)
- watchdog-timeout settle path never releases the cleanup-detach exemption (attachment-lifecycle.test.ts) — already recorded as round-5/6 convergence-posture deferrals (reviews 5029145840, 5032096362)
- watchdog test release-before-callback ordering unpinned (attachment-lifecycle.test.ts) — already recorded as round-2/3/5/6/7/8 convergence-posture deferrals (reviews 5013080184, 5014440443, 5029145840, 5032096362, 5045295751, 5051873342)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 18: none — no checks were cut short..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/transport-safety.test.ts:334 — [probe] runRequest reservation identity unpinned — a structural-clone reservation mutant ships green (865 tests) while object-response charges leak until the queue limit retires a healt…packages/acp-bridge/src/transport-safety.ts:491 — [probe] log-safe mask errorKind/status/hint preservation contract unpinned — the mask-destroying mutant ships green (804+18 tests) while child errorKind-keyed fallbacks silently stop firing
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 18：none — no checks were cut short.。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 1a": could not run tsc --noEmit or the webui vitest suite against the commit — the review worktree has no node_modules and a full monorepo install was not attempte….
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/webui/src/daemon/session/DaemonSessionProvider.tsx:1619 — [probe] R9-1 guard wiring unpinned at the provider layerpackages/webui/src/daemon/session/DaemonSessionProvider.tsx:1775 — [review] R9-1 comment misnames the adoption bypass dependentpackages/webui/src/daemon/session/DaemonSessionProvider.tsx:1773 — [review] attemptedLoadForRun is iteration-scoped, not run-scoped
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 1a"：could not run tsc --noEmit or the webui vitest suite against the commit — the review worktree has no node_modules and a full monorepo install was not attempte…。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R11-1 R9-1 provider-layer guard wiring unpinned (DaemonSessionProvider.tsx:1780) — already recorded as the round-10 convergence-posture deferral (review 5055463393)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Maintainer verification — built a real local environment and A/B'd every seam against the merge-baseVerified at head Verdict: the behavior-preserving claim holds. Approving. Two of the deltas are not just acceptable — they fix live bugs on
1. The inbound estimator (deferred call #1) — regression is genuinely fixed, and the outbound side fixes a
|
| property | result |
|---|---|
| values the new estimator refuses | 0 / 200,000 |
byte under-counts vs real JSON.stringify |
0 / 200,000 |
| verdict differs from the old inbound estimator | 0 / 200,000 |
| verdict differs from the old outbound estimator | 50,831 / 200,000 — all in the admit direction |
So R1-1 is fixed, at scale, not just for the one shape in the regression test. 27 hand-built adversarial shapes agree (cycles, shared DAGs, getters, setters, callable and accessor toJSON, Date, class instances, bigint, symbol keys, non-enumerable own keys).
The structural argument is stronger than the sampling: estimateTransportValueBytes is the old outbound estimator with Object.hasOwn(v, 'toJSON') replaced by hasStructuralToJSONHazard(v), which is a strict subset of it. The change is therefore monotone — it can only ever refuse fewer values, never mis-size one.
The outbound relaxation is reachable, and it fixes a live availability bug on main. I drove a real createSpawnChannelFactory spawning a real NDJSON child, wired through the real ClientSideConnection + createLogSafeAcpClient + createOutboundGuardedConnection exactly as bridge.ts does:
| scenario | merge-base | PR head |
|---|---|---|
inbound session/update carrying a plain-data toJSON |
alive | alive |
inbound fs/read_text_file carrying a data toJSON |
alive | alive |
prepared response carrying a data toJSON |
transport retired, child SIGTERM'd (exit 143) | alive |
client-hosted MCP tool result forwarded via extMethod |
transport retired, child SIGTERM'd (exit 143) | alive |
That last row is the production shape, not a synthetic one: BridgeClient.handleClientMcpMessage ends in return { payload: response }, where response is the JSON-RPC reply an extension's client-hosted MCP server sent back over the daemon WebSocket. On main, an MCP tool result containing a member literally named toJSON — a perfectly legal JSON key, and one an extension author controls — raises NdJsonQueueLimitError, destroys stdin/stdout and terminates the agent child. This PR is what stops that.
I'd call this a fix worth naming in the PR body rather than leaving it filed as "a policy change embedded in the move".
Non-blocking residual, pre-existing on both arms: both estimators only inspect own properties, so Object.prototype.toJSON pollution under-counts (measured: est 30 vs real 500,002 bytes). main has the identical blind spot via Object.hasOwn. Not introduced here; noting it so it is on the record.
2. releaseCleanupDetachExemption(undefined) (deferred call #2) — unreachable, and the new guard is stricter than main
The unconditional-clear path the bot flagged is not reachable from production code. releaseCleanupDetachExemption has exactly one non-test call site (actions.ts:572), and it passes both currentSession and pendingLoad. The optional-parameter shape is API surface on a new internal class, not a live behavior.
More importantly, the PR's guard is a strict subset of main's clears, and closes a real gap. Main compares session identity only (skipNextCleanupDetachSessionRef.current === currentSession), which cannot distinguish two overlapping reloads of the same session, because they share the session object. Runtime A/B of that exact interleaving:
two overlapping reloads of the SAME session; the superseded load's late .finally() releases
MERGE-BASE exemption still held for the in-flight reload B: NO <-- superseded load cleared the successor
PR-HEAD exemption still held for the in-flight reload B: YES
Also pinned, since it was recorded as a deferred probe in an earlier round: the watchdog path does release the pending load before invoking onTimeout (pendingLoad observed as undefined from inside the callback). And stale-load isolation holds — a superseded load cannot settle the current attachment; supersede rejects it with AbortError.
3. Everything else, A/B'd rather than read
Seam 2 — workspace-qualified trusted routes. Booted two real daemons (node packages/cli/dist/index.js serve), folder trust enabled, two registered workspaces — one TRUST_FOLDER, one DO_NOT_TRUST. Probed 80 workspace-qualified routes × 6 selectors (registered id, absolute path, unknown id, non-absolute garbage; trusted and untrusted), plus 9 well-formed requests against the untrusted workspace so the trust gate is reached rather than short-circuited by param validation. 489 probes, 0 differences in status code or error code. 63/80 routes answer 403 untrusted_workspace; 76/80 answer 400 workspace_mismatch for an unknown selector — identically on both arms. This goes past the CI Serve A/B's 12 scenarios in the direction that matters for this seam: the negative and trust paths.
Unchanged-by-this-PR observation: /file, /file/bytes, /stat, /list, /glob answer 200 on an untrusted workspace on both arms. That is pre-existing read-only-browsing policy, not something this PR introduces — flagging only so the 200s in my matrix are not misread.
Seam 3 — CLI shim deletion. Enumerated the export surface of packages/cli/src/serve/index.ts on both arms with the TypeScript compiler API (no module execution, no build dependency): 120 symbols, identical, zero diff. No file still imports acp-session-bridge.
Seam 4 — Web Shell prompt admission. The 8 extracted helpers (retryOwnerMatchesCurrent, getLatestUserBlock, matchesUserMessageIdentity, findUserMessageByIdentity, getRetryableTurnError, matchesTurnErrorIdentity, retryTranscriptIdentityMatches, getLatestUserBlockId) are faithful moves from App.tsx: 7 of the 8 are byte-identical, and getRetryableTurnError differs only by an i → index loop-variable rename plus a single-statement brace collapse. For PromptAdmissionAttempt I transcribed main's branch expressions and ran an exhaustive truth table over (started × admitted × definitelyRejected): 8/8 states produce identical branch decisions for the unknown-admission branch, the restore branch and the not-started guard. All four call sites checked individually — including the multi-line uncertain-admission condition at the sendPrompt site, which folds into classifyFailure(...) === 'unknown' correctly. The ownership gate is evaluated live at each guard, so a retry cannot commit after a session switch.
Seam 5 — WebUI attachment lifecycle. Covered above.
Gates. npm run typecheck across the whole repo: 11 errors on merge-base, the identical 11 on PR head (all in @qwen-code/external-context-mem0, an ajv import-style drift; untouched by this PR). Worth stating explicitly because ci.yml does not run tsc, so this is the one gate a PR of this size could otherwise slip past. Unit tests on PR head: acp-bridge 1812/1812, web-shell 4423/4423, webui 651/651.
4. The web-shell visual render failures are not caused by this PR
This was the open question in the deferral. I reproduced the CI symptom locally and ran the same suite on both arms:
| merge-base | PR head | |
|---|---|---|
| web-shell visual capture (37 scenarios) | 35 pass / 2 fail | 35 pass / 2 fail |
Same two scenarios, same error, on both arms — workspace sidebar (dark and light), failing with:
strict mode violation: getByRole('complementary').getByText('Run auth migration')
resolved to 2 elements
That is a selector-strictness bug in packages/web-shell/client/e2e/visuals/screenshots.spec.ts:778 — the loaded session's name renders both in the attention group and in its per-workspace group, so the non-exact getByText matches twice. The PR touches no file under client/e2e/. No /verify needed; this is a pre-existing failure on main that suppresses the visuals preview for every PR touching web-shell, and it deserves its own small fix.
Summary
| seam | claim | verdict |
|---|---|---|
| 1 · bounded ACP transport | behavior-preserving | ✅ inbound preserved (0/200k diffs); outbound relaxation is monotone, sound, and fixes a live child-kill bug on main |
| 2 · trusted workspace routes | behavior-preserving | ✅ 489 live probes, 0 differences |
| 3 · CLI shim deletion | public exports preserved | ✅ 120 symbols identical |
| 4 · prompt admission | behavior-preserving | ✅ faithful moves + 8/8 exhaustive truth table |
| 5 · attachment lifecycle | behavior-preserving | ✅ preserved, and the exemption guard is strictly tighter than main |
Two follow-ups, neither blocking this PR:
- Fix
screenshots.spec.ts:778's ambiguous locator so the visuals preview stops being suppressed repo-wide. - Consider calling out the MCP-tool-result transport-kill fix in the PR body — it is a user-visible fix hiding inside a refactor, and it is the kind of thing worth finding in
git loglater.
On size: 2,959 production lines across four packages in one PR is a lot to revert as a unit, and the design doc's five stages really were five landable PRs. I'm merging this one, but let's land the next pass staged.
中文版报告
维护者验证 —— 已在本地搭建真实环境，逐个接缝与 merge-base 做 A/B
在 head 5be8b5dd43 与 merge-base 4ef780cbfa 上验证，Linux x86_64 / Node v22.22.2，两侧分别在独立 worktree 中完整构建。重点攻了 triage 机器人转交给我的两个判断点，外加 web-shell 视觉渲染失败这个悬而未决的问题。
结论：行为保持的说法成立，予以合并。 其中两处 delta 不只是可以接受 —— 它们修掉了 main 上的真实 bug。
1. 入站估算器（转交判断点 #1）—— 回归确已修复，且出站侧修掉了 main 的一个 bug
我把三个估算器全部抽出来并排跑：merge-base 宽松的入站 estimateAcpHandlerBytes、merge-base 严格的出站 estimatePreparedResponseBytes，以及本 PR 统一后的 estimateTransportValueBytes，用真实 JSON.stringify 作为 oracle。
对 20 万条 wire 形态的值（生成后经 JSON.stringify/JSON.parse 往返，即入站路径实际收到的形态）：
| 指标 | 结果 |
|---|---|
| 新估算器拒绝的值 | 0 / 200,000 |
相对真实 JSON.stringify 的字节低估 |
0 / 200,000 |
| 与旧入站估算器判定不一致 | 0 / 200,000 |
| 与旧出站估算器判定不一致 | 50,831 / 200,000 —— 全部朝“放行”方向 |
即 R1-1 确实被修复了，而且是在规模上成立，不只是回归测试里那一个形态。另有 27 个手工构造的对抗形态（循环、共享 DAG、getter、setter、可调用与访问器 toJSON、Date、类实例、bigint、symbol 键、不可枚举自有键）结论一致。
结构性论证比抽样更强：estimateTransportValueBytes 就是旧出站估算器把 Object.hasOwn(v, 'toJSON') 换成了 hasStructuralToJSONHazard(v)，而后者是前者的真子集。因此这个改动是单调的 —— 只可能拒绝更少的值，不可能把某个值的大小算错。
出站放宽是可达的，而且它修掉了 main 上一个真实的可用性 bug。 我驱动了真实的 createSpawnChannelFactory 去 spawn 一个真实的 NDJSON 子进程，并完全按 bridge.ts 的方式接上真实的 ClientSideConnection + createLogSafeAcpClient + createOutboundGuardedConnection：
| 场景 | merge-base | PR head |
|---|---|---|
入站 session/update 携带纯数据 toJSON |
存活 | 存活 |
入站 fs/read_text_file 携带数据 toJSON |
存活 | 存活 |
prepared response 携带数据 toJSON |
传输被退休，子进程被 SIGTERM（exit 143） | 存活 |
客户端托管 MCP 工具结果经 extMethod 转发 |
传输被退休，子进程被 SIGTERM（exit 143） | 存活 |
最后一行是生产形态而非人为构造：BridgeClient.handleClientMcpMessage 的结尾是 return { payload: response }，其中 response 是扩展所托管的 MCP server 经 daemon WebSocket 回传的 JSON-RPC 应答。在 main 上，只要某个 MCP 工具结果里含有一个字面名为 toJSON 的成员 —— 这是完全合法的 JSON 键，且由扩展作者控制 —— 就会抛出 NdJsonQueueLimitError，销毁 stdin/stdout 并终止 agent 子进程。是这个 PR 止住了它。
我认为这值得在 PR 正文里明确写成一处修复，而不是仅仅归档为「搬移中夹带的策略变更」。
补充（非阻塞，两侧均已存在）：两个估算器都只检查自有属性，所以 Object.prototype.toJSON 污染会导致低估（实测：估算 30 字节 vs 实际 500,002 字节）。main 经由 Object.hasOwn 有完全相同的盲点。并非本 PR 引入，仅作记录。
2. releaseCleanupDetachExemption(undefined)（转交判断点 #2）—— 不可达，且新守卫比 main 更严格
机器人标记的无条件清除路径在生产代码中不可达。releaseCleanupDetachExemption 只有一个非测试调用点（actions.ts:572），且两个参数都传了实值（currentSession 与 pendingLoad）。可选参数只是新内部类的 API 形状，不构成实际行为。
更重要的是，PR 的守卫是 main 清除集合的真子集，并且补上了一个真实缺口。main 只比较 session 身份（skipNextCleanupDetachSessionRef.current === currentSession），无法区分同一个 session 的两次重叠 reload，因为它们共享同一个 session 对象。对这一交错场景的运行时 A/B：
同一 session 的两次重叠 reload；被取代的那次 load 的迟到 .finally() 执行释放
MERGE-BASE 仍在飞行中的 reload B 的豁免是否保留：否 <-- 被取代的 load 清掉了后继者的豁免
PR-HEAD 仍在飞行中的 reload B 的豁免是否保留：是
另外，因为前几轮把它记成了 deferred probe，这里一并钉住：watchdog 路径确实在调用 onTimeout 之前释放了 pending load（在回调内部观测到 pendingLoad 为 undefined）。stale-load 隔离同样成立 —— 被取代的 load 无法结算当前 attachment，取代动作会以 AbortError reject 它。
3. 其余部分：用 A/B 验证，而非阅读代码
接缝 2 —— workspace-qualified 可信路由。 启动两个真实 daemon（node packages/cli/dist/index.js serve），开启 folder trust，注册两个 workspace —— 一个 TRUST_FOLDER，一个 DO_NOT_TRUST。探测 80 条 workspace-qualified 路由 × 6 种选择子（已注册 id、绝对路径、未知 id、非绝对路径垃圾值；可信与不可信各一组），另加 9 个针对不可信 workspace 的格式完整请求，以确保真正触达 trust 门而不是被参数校验提前短路。489 次探测，状态码与错误码 0 差异。 63/80 条路由返回 403 untrusted_workspace；对未知选择子 76/80 条返回 400 workspace_mismatch —— 两侧完全一致。这比 CI Serve A/B 的 12 个场景更进一步，且进的正是这个接缝该覆盖的方向：否定路径与信任路径。
补充（与本 PR 无关）：/file、/file/bytes、/stat、/list、/glob 在两侧都对不可信 workspace 返回 200。这是既有的只读浏览策略，并非本 PR 引入 —— 特此说明，以免我矩阵里的 200 被误读。
接缝 3 —— CLI 兼容层删除。 用 TypeScript 编译器 API 在两侧枚举 packages/cli/src/serve/index.ts 的导出面（不执行模块、不依赖构建）：120 个符号，完全一致，零差异。已无任何文件 import acp-session-bridge。
接缝 4 —— Web Shell prompt admission。 抽出的 8 个 helper（retryOwnerMatchesCurrent、getLatestUserBlock、matchesUserMessageIdentity、findUserMessageByIdentity、getRetryableTurnError、matchesTurnErrorIdentity、retryTranscriptIdentityMatches、getLatestUserBlockId）是从 App.tsx 忠实搬移：其中 7 个逐字节一致，getRetryableTurnError 的差异仅为 i → index 循环变量重命名，外加一处单语句大括号折叠。对 PromptAdmissionAttempt，我把 main 的分支表达式转写出来，做了 (started × admitted × definitelyRejected) 的穷举真值表：8/8 个状态下，unknown 分支、restore 分支与 not-started 守卫的判定完全一致。四处调用点逐个核对，包括 sendPrompt 处那个多行的 uncertain-admission 条件，它折叠成 classifyFailure(...) === 'unknown' 是正确的。所有权门在每个守卫处实时求值，因此用户切换 session 后 retry 无法提交。
接缝 5 —— WebUI attachment lifecycle。 见上文。
各项门禁。 全仓 npm run typecheck：merge-base 11 个错误，PR head 同样的 11 个（全部位于 @qwen-code/external-context-mem0，属 ajv import 风格漂移；本 PR 未触碰）。之所以要明确写出来，是因为 ci.yml 并不跑 tsc，对这种体量的 PR 而言这是唯一可能被绕过的门禁。PR head 单测：acp-bridge 1812/1812、web-shell 4423/4423、webui 651/651。
4. web-shell 视觉渲染失败不是本 PR 造成的
这是转交时留下的开放问题。我在本地复现了 CI 症状，并在两侧跑了同一套用例：
| merge-base | PR head | |
|---|---|---|
| web-shell 视觉截图（37 个场景） | 35 通过 / 2 失败 | 35 通过 / 2 失败 |
两侧是同样的两个场景、同样的报错 —— workspace sidebar（dark 与 light），报错为：
strict mode violation: getByRole('complementary').getByText('Run auth migration')
resolved to 2 elements
这是 packages/web-shell/client/e2e/visuals/screenshots.spec.ts:778 的选择器严格性 bug —— 已加载 session 的名称同时渲染在 attention 分组和它所属的 workspace 分组里，因此非 exact 的 getByText 匹配到了两个。本 PR 未触碰 client/e2e/ 下的任何文件。无需 /verify；这是 main 上既有的失败，它会让每个改动 web-shell 的 PR 都拿不到视觉预览，值得单独修一下。
小结
| 接缝 | 声称 | 结论 |
|---|---|---|
| 1 · 有界 ACP 传输 | 行为保持 | ✅ 入站保持（20 万条 0 差异）；出站放宽单调、可靠，且修掉 main 上一个会杀死子进程的真实 bug |
| 2 · 可信 workspace 路由 | 行为保持 | ✅ 489 次实况探测，0 差异 |
| 3 · CLI 兼容层删除 | 公共导出不变 | ✅ 120 个符号完全一致 |
| 4 · prompt admission | 行为保持 | ✅ 忠实搬移 + 8/8 穷举真值表 |
| 5 · attachment lifecycle | 行为保持 | ✅ 保持，且豁免守卫比 main 更严格 |
两个后续项，均不阻塞本 PR：
- 修掉
screenshots.spec.ts:778的歧义选择器，让视觉预览不再被全仓性地压制。 - 考虑在 PR 正文中点名 MCP 工具结果导致传输被杀这处修复 —— 这是藏在重构里的用户可见修复，日后在
git log里能被找到才有价值。
关于体量：一个 PR 跨 4 个 package、2,959 行生产代码，作为一个整体回滚的代价偏高，设计文档里的五个阶段本来确实可以拆成五个 PR。这一个我合了，但下一轮请分阶段落地。
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 115 passed · 1 failed · 116 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：115 通过 · 1 失败 · 116 总计 抖动门： Verification reportPR 9682 verification — refactor: deepen architecture ownership boundariesVerdict: 中文摘要
Central claim + A/BCentral claim: the transport-safety centralization is behavior-preserving — Method: head arm = compiled
The base lenient estimator's census (admits callable toJSON, calls accessor Secondary claims verified:
FindingsF1 (Suggestion) — R9-1's attempted-load guard is not pinned by any testReverting the guard added in the R9-1 commit — replacing Reproduce: cd packages/webui && python3 - <<'EOF'
F='src/daemon/session/DaemonSessionProvider.tsx'; s=open(F,encoding='utf-8').read()
open(F,'w',encoding='utf-8').write(s.replace("(attemptedLoadForIteration === undefined ||\n pendingLoad === attemptedLoadForIteration)","true"))
EOF
npx vitest run src/daemon/session/DaemonSessionProvider.test.tsx src/daemon/session/DaemonSessionProvider.subagent.test.ts
# then restore the fileCaveat on the sweep: the web-shell suite (4449 tests) also stayed green, but F2 (observation, pre-existing, not introduced by this PR) — NUL bytes in DaemonSessionProvider.tsx
F3 (completeness, not a merge condition) — R6-1 pinning set is a pairWith the load-association guard deleted (W1), only 1 of the 3 successor Mutation matrix
Targeted gates (all green at head)
Not covered
MethodologyEnvironment: CI merge-ref checkout ( Flakiness gate logEvidence 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. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
…overnance # Conflicts: # packages/cli/src/serve/acp-session-bridge.ts
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.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R12-1 R9-1 adoption guard unpinned (DaemonSessionProvider.tsx:1786) — already reported as the round-10 convergence-posture deferral (review 5055463393) and round-11 R11-1
- R12-2 watchdog test release-before-callback ordering unpinned (attachment-lifecycle.test.ts:99) — already reported as round-2/3/5/6/7/8 convergence-posture deferrals (reviews 5013080184, 5014440443, 5029145840, 5032096362, 5045295751, 50518…
- R12-3 notification no-reservation invariant unpinned (transport-safety.test.ts:243) — already reported as round-5/6 convergence-posture deferrals (reviews 5029145840, 5032096362)
- R12-4 watchdog onTimeout swallow / load-always-settles guarantee unpinned (attachment-lifecycle.ts:73) — already reported as the round-6 deferral (review 5032096362) and round-8 R8-3
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/prompt-admission.ts:150 — [probe] turn-error selection and identity helpers unpinned by any testdocs/developers/daemon/03-acp-bridge.md:269 (+1 locations) — [probe] added doc sentences overstate the subpath migrationpackages/webui/src/daemon/session/attachment-lifecycle.test.ts:285 — [probe] resolve/reject cleanup-detach exemption asymmetry unpinnedpackages/acp-bridge/src/transport-safety.ts:86 — [probe] retained-message count arm unpinned in both budgetspackages/acp-bridge/src/transport-safety.ts:135 — [probe] outbound overflow rejection untested
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — did not converge within the reverse-audit round cap of 3。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Maintainer verification, round 2 — re-built the environment at the new head
|
| state at the guard | evaluations |
|---|---|
| pendingLoad's session ≠ active session → guard irrelevant | 365 |
| session matches, iteration captured the same load → guard passes | 20 |
| session matches, iteration captured no load → the documented creation-flow bypass | 5 |
| discriminating — captured load ≠ current pendingLoad | 0 / 390 |
The branch the guard exists for is never entered by any existing test. So I built the fixture that enters it. The interleaving is: a restore is in flight; the successor's startPendingLoad runs synchronously inside the action, while the stale run's continuation is a microtask that was queued first — so the stale run wakes up past its abort check with pendingLoad already replaced.
| arm | guard | result |
|---|---|---|
PR head 1422d79b22 |
present | ✅ passes |
PR head, guard → true |
reverted | ❌ expected 'resolved' to be undefined |
merge-base 5c47cf95bc (i.e. main) |
absent | ❌ expected 'resolved' to be undefined |
That third row is the point. The fixture only drives the public actions surface (resumeSession), so it drops onto main unchanged — and fails there. On main, pendingLoadToResolve is keyed on session identity alone:
// main
const pendingLoadToResolve =
pendingLoad?.sessionId === activeSession.sessionId ? pendingLoad : undefined;Two overlapping restores of the same session share the session object, so a superseded run resolves the successor's pending load. User-visible shape: the newer session switch reports itself complete while its own load/resume is still in flight — the caller's promise settles on a restore that was never theirs.
So this is a second live-bug fix hiding inside the refactor, alongside the outbound-estimator one I flagged last time. Worth naming in the PR body.
The fixture, ready to paste into DaemonSessionProvider.test.tsx (whole packages/webui suite with it added: 655/655, and repo typecheck clean):
R9-1 pinning fixture
// R9-1 regression fixture: a superseded restore run must not settle the
// successor's pending load. The stale run's `client.resume()` settles in the
// microtask queued BEFORE the successor registers its own load, so the stale
// run reaches the adoption point while `attachmentLifecycle.pendingLoad` is
// already the successor's. Without the attempted-load guard it resolves the
// successor's promise even though the successor's own restore never settled.
it('does not let a superseded restore settle the successor pending load', async () => {
vi.stubGlobal(
'fetch',
vi.fn(async () => new Response(null, { status: 204 })),
);
const sessionA = createMockSession({
sessionId: 'session-a',
events: createIdleEvents(),
});
sdkMocks.sessions.push(sessionA);
let actions: DaemonSessionActions | undefined;
function Harness() {
actions = useDaemonActions();
return null;
}
await renderWithProvider(<Harness />, { autoConnect: true });
await act(async () => {
await flushPromises();
});
const staleRestore = createDeferred<MockSession>();
const neverSettles = createDeferred<MockSession>();
sdkMocks.MockDaemonSessionClient.resume
.mockImplementationOnce(async () => staleRestore.promise)
.mockImplementationOnce(async () => neverSettles.promise);
let firstOutcome: string | undefined;
let secondOutcome: string | undefined;
act(() => {
void requireActions(actions)
.resumeSession('session-b')
.then(
() => {
firstOutcome = 'resolved';
},
() => {
firstOutcome = 'rejected';
},
);
});
await act(async () => {
await flushPromises();
});
const sessionB = createMockSession({
sessionId: 'session-b',
events: createIdleEvents(),
replaySnapshot: createTextReplaySnapshot('session b transcript'),
});
await act(async () => {
// Queue the stale run's continuation first, then register the successor
// load synchronously — the stale run wakes up with pendingLoad already
// replaced.
staleRestore.resolve(sessionB);
void requireActions(actions)
.resumeSession('session-b')
.then(
() => {
secondOutcome = 'resolved';
},
() => {
secondOutcome = 'rejected';
},
);
await flushPromises();
await flushPromises();
await flushPromises();
});
// The first switch was superseded, so it rejects with AbortError.
expect(firstOutcome).toBe('rejected');
// The successor's own restore never settled, so nothing may settle it.
expect(secondOutcome).toBeUndefined();
await act(async () => {
neverSettles.reject(new Error('cleanup'));
root?.unmount();
root = null;
await flushPromises();
});
});2. The merge is clean — checked, not assumed
main moved 15 commits under this branch since my last pass, three of them touching files this PR rewrites. All three survived the merge intact:
main commit |
state at head |
|---|---|
#9955 fix(core): expose resolved agent execution mode |
present — isTaskExecutionMode wired into the provider's subagent projection |
#9895 feat(daemon): support scoped workspace memory tasks |
present — workspace-remember.ts carries main's version |
#9406 feat(serve): hide workspace Browse on headless daemon hosts |
present — nativeDirectoryPickerSupported kept in App.tsx, alongside the PR's own extraction from the same file (+59/−197 against the merge-base) |
One thing the merge does drop, correctly: main added BridgeWorkspaceMemoryRememberTargetScope to the re-export list in acp-session-bridge.ts, the shim this PR deletes. That symbol was never part of the public serve/index.ts barrel, and workspace-remember.ts imports it straight from @qwen-code/acp-bridge/bridgeTypes at head. Nothing references the deleted file.
Also checked for the failure mode a refactor of this shape invites: none of the 8 extracted admission helpers is left behind as a duplicate definition in App.tsx.
3. Every seam re-measured at the new pair
Seam 1 — bounded ACP transport. transport-safety.ts is byte-identical to the head I verified before, so the estimator logic didn't move; I re-ran the differential anyway at the new pair. 200,000 wire-shaped values (generated, then round-tripped through JSON.stringify/JSON.parse), with real JSON.stringify as the oracle and both of main's estimators exported from the merge-base worktree whose only diff is two export keywords: 0 refusals, 0 byte under-counts, 0 verdict differences against main's inbound estimator, and 58,310 differences against main's strict outbound estimator — all in the admit direction. acp-bridge: 1812/1812.
Seam 2 — workspace-qualified trusted routes. This is where main's new commits landed, so it got the full treatment again rather than a carry-forward. Two real daemons (node dist/cli.js serve), folder trust on, one TRUST_FOLDER and one DO_NOT_TRUST workspace registered per arm, each arm with its own workspace pair so the mutating probes can't cross-contaminate. 80 workspace-qualified routes × 6 selectors = 480 probes per arm, 0 differences in status code or error code. 63 routes answer 403 untrusted_workspace; 76 answer 400 workspace_mismatch for an unknown or non-absolute selector. I added a runtime-readiness gate before measuring — without it the daemon answers 503 daemon_runtime_starting for the first second and the whole matrix is meaningless.
Seam 3 — CLI shim deletion. Export surface of packages/cli/src/serve/index.ts enumerated with the TypeScript compiler API on both arms: 120 symbols, identical, zero diff.
Seam 4 — Web Shell prompt admission. prompt-admission.test.ts 5/5. Full web-shell suite: head 4387 passed, base 4382 passed (+5 = the new file). 8 files fail identically on both arms in my link-farm environment (duplicate-React resolution and one un-built lib artifact) — same file set, same per-file counts, so not attributable to this PR.
Seam 5 — WebUI attachment lifecycle. Section 1.
Gates. Repo-wide npm run typecheck: 16 errors on the merge-base, the identical 16 on head (11 are the ajv import drift in @qwen-code/external-context-mem0, 3 are pre-existing web-shell errors, 2 are an unbuilt audio-capture dist in my tree). Worth restating because ci.yml does not run tsc. packages/cli serve suites touching this PR: 1405/1405.
Summary
| seam | verdict at 1422d79b22 |
|---|---|
| 1 · bounded ACP transport | ✅ inbound preserved (0/200k), outbound relaxation monotone and sound |
| 2 · trusted workspace routes | ✅ 480 live probes per arm, 0 differences |
| 3 · CLI shim deletion | ✅ 120 exported symbols identical |
| 4 · prompt admission | ✅ faithful extraction, no duplicate definitions left behind |
| 5 · attachment lifecycle | ✅ preserved — and the R9-1 guard fixes a defect present on main |
| merge integrity | ✅ all three main commits in PR-touched files preserved |
Merging. Follow-ups, none blocking:
- Land the R9-1 fixture above (or your own equivalent) — right now the only thing standing between that guard and a future "this looks dead, delete it" is this comment.
- Name both live-bug fixes in the PR body: the MCP-tool-result transport kill, and this superseded-load resolution. Both are user-visible fixes filed as refactor.
- Still standing from last round:
screenshots.spec.ts:778's ambiguous locator suppresses the visuals preview repo-wide. - And the size point stands — next pass, land it staged.
中文版报告
维护者验证（第二轮）—— 在新 head 1422d79b22 上重建了环境
上一轮之后 head 发生了变化（5be8b5dd43 → 1422d79b22，一次与 main 的新合并），同时沙箱 /triage 报了一个未解决的 finding。所以我重建了两侧环境 —— PR head 1422d79b22 与其 merge-base 5c47cf95bc，各自在独立 worktree 中完整构建，Linux x86_64 / Node v22.22.2 —— 并重点做三件事：(1) 攻沙箱的 F1；(2) 检查这次 merge 是否悄悄丢掉了 main 落在本 PR 所改文件里的东西；(3) 在新的这对提交上把每个接缝的 A/B 重跑一遍，而不是沿用上一轮的数字。
结论：依然合入。而 F1 反倒是这个 PR 上最有价值的发现 —— 那个没有测试钉住的守卫不只是活代码，它修掉的是 main 上此刻就存在的一个 bug。下面给出 fixture，并做了三方向验证。
1. F1 结案 —— R9-1 守卫是承重代码，且 main 上存在它所防的缺陷
沙箱报告说：把 DaemonSessionProvider.tsx 中的 attemptedLoadForIteration 守卫回退掉，provider 套件依然全绿。在这个 head 上确认属实，而且范围更大：把守卫换成 true 后，整个 packages/webui 套件（36 个文件、654 个测试）仍然全绿。所以这是真实的覆盖缺口，不是沙箱假象。
随后我在真实 provider 的判定点埋了探针，跑完整套件，看为什么没有测试能杀掉它：
| 判定点处的状态 | 求值次数 |
|---|---|
| pendingLoad 的 session ≠ 当前 session → 守卫无关 | 365 |
| session 匹配，本轮迭代捕获的是同一个 load → 守卫放行 | 20 |
| session 匹配，本轮迭代没有捕获 load → 注释中说明的创建流旁路 | 5 |
| 判别态 —— 捕获的 load ≠ 当前 pendingLoad | 0 / 390 |
守卫存在的那个分支，现有测试从未进入过。于是我把进入该分支的 fixture 构造了出来。交错时序是：一次 restore 正在飞行中；后继者的 startPendingLoad 在 action 内部同步执行，而过期那次运行的续体是一个更早入队的 microtask —— 因此过期运行会越过自己的 abort 检查醒来，此时 pendingLoad 已经被替换。
| 臂 | 守卫 | 结果 |
|---|---|---|
PR head 1422d79b22 |
存在 | ✅ 通过 |
PR head，守卫改为 true |
回退 | ❌ expected 'resolved' to be undefined |
merge-base 5c47cf95bc（即 main） |
没有 | ❌ expected 'resolved' to be undefined |
关键是第三行。这个 fixture 只驱动公开的 actions 接口（resumeSession），所以可以原样放到 main 上 —— 而它在 main 上失败。main 的 pendingLoadToResolve 仅以 session 身份为键：
// main
const pendingLoadToResolve =
pendingLoad?.sessionId === activeSession.sessionId ? pendingLoad : undefined;同一 session 的两次重叠 restore 共享同一个 session 对象，因此被取代的那次运行会结算后继者的 pending load。用户可见的表现是：较新的那次 session 切换在自己的 load/resume 仍在飞行中时就报告完成 —— 调用方的 promise 结算在了一次并不属于它的 restore 上。
所以这是藏在重构里的第二处真实修复，与上一轮我指出的出站估算器那处并列。值得在 PR 正文中点名。
fixture 可直接粘进 DaemonSessionProvider.test.tsx（加入后 packages/webui 全套 655/655），见上文英文部分的折叠块。
2. 这次 merge 是干净的 —— 是查过的，不是假设的
自上一轮以来 main 在本分支下推进了 15 个提交，其中 3 个触及了本 PR 重写的文件。三个都完整保留了下来：
main 提交 |
head 上的状态 |
|---|---|
#9955 fix(core): expose resolved agent execution mode |
在 —— isTaskExecutionMode 已接入 provider 的 subagent 投影 |
#9895 feat(daemon): support scoped workspace memory tasks |
在 —— workspace-remember.ts 保留了 main 的版本 |
#9406 feat(serve): hide workspace Browse on headless daemon hosts |
在 —— nativeDirectoryPickerSupported 保留在 App.tsx 中，与本 PR 对同一文件的抽取并存（相对 merge-base +59/−197） |
merge 确实丢掉了一样东西，而且丢得正确：main 往本 PR 删除的兼容层 acp-session-bridge.ts 的 re-export 列表里加了 BridgeWorkspaceMemoryRememberTargetScope。该符号从来就不在公开的 serve/index.ts barrel 里，且 head 上 workspace-remember.ts 直接从 @qwen-code/acp-bridge/bridgeTypes 导入它。已无任何文件引用被删掉的那个文件。
另外也检查了这种形态的重构最容易出的问题：8 个被抽出的 admission helper，没有任何一个在 App.tsx 里残留重复定义。
3. 每个接缝都在新的这对提交上重新测量
接缝 1 —— 有界 ACP 传输。 transport-safety.ts 与我上一轮验证的 head 逐字节一致，估算器逻辑没有移动；但我还是在新的这对提交上重跑了差分。20 万条 wire 形态的值（生成后经 JSON.stringify/JSON.parse 往返），以真实 JSON.stringify 为 oracle，main 的两个估算器从 merge-base worktree 导出（该 worktree 的唯一 diff 是两个 export 关键字）：0 拒绝、0 字节低估、与 main 入站估算器 0 判定差异，与 main 严格出站估算器有 58,310 处差异 —— 全部朝放行方向。acp-bridge：1812/1812。
接缝 2 —— workspace-qualified 可信路由。 main 的新提交正落在这里，所以这一块是完整重做而不是沿用。两个真实 daemon（node dist/cli.js serve），开启 folder trust，每侧各注册一个 TRUST_FOLDER 和一个 DO_NOT_TRUST workspace，且两侧各用独立的 workspace 目录，避免带写操作的探测互相污染。80 条 workspace-qualified 路由 × 6 种选择子 = 每侧 480 次探测，状态码与错误码 0 差异。 63 条返回 403 untrusted_workspace；对未知或非绝对路径选择子，76 条返回 400 workspace_mismatch。我在测量前加了运行时就绪门 —— 没有它的话，daemon 头一秒会一律返回 503 daemon_runtime_starting，整个矩阵就毫无意义了。
接缝 3 —— CLI 兼容层删除。 用 TypeScript 编译器 API 在两侧枚举 packages/cli/src/serve/index.ts 的导出面：120 个符号，完全一致，零差异。
接缝 4 —— Web Shell prompt admission。 prompt-admission.test.ts 5/5。web-shell 全套：head 4387 通过，base 4382 通过（+5 即新增文件）。有 8 个文件在两侧都以完全相同的方式失败（我这套 link-farm 环境的 duplicate-React 解析问题，以及一个未构建的 lib 产物）—— 相同文件集、相同计数，因此不归因于本 PR。
接缝 5 —— WebUI attachment lifecycle。 见第 1 节。
各项门禁。 全仓 npm run typecheck：merge-base 16 个错误，head 同样的 16 个（11 个是 @qwen-code/external-context-mem0 的 ajv import 漂移，3 个是 web-shell 既有错误，2 个是我这棵树里 audio-capture 未构建的 dist）。之所以重申，是因为 ci.yml 并不跑 tsc。packages/cli 中与本 PR 相关的 serve 套件：1405/1405。
小结
| 接缝 | 在 1422d79b22 上的结论 |
|---|---|
| 1 · 有界 ACP 传输 | ✅ 入站保持（0/20 万），出站放宽单调且可靠 |
| 2 · 可信 workspace 路由 | ✅ 每侧 480 次实况探测，0 差异 |
| 3 · CLI 兼容层删除 | ✅ 120 个导出符号完全一致 |
| 4 · prompt admission | ✅ 忠实抽取，未残留重复定义 |
| 5 · attachment lifecycle | ✅ 保持 —— 且 R9-1 守卫修掉了 main 上存在的一个缺陷 |
| merge 完整性 | ✅ 落在本 PR 所改文件里的 3 个 main 提交全部保留 |
合并。 后续项，均不阻塞：
- 把上面那个 R9-1 fixture（或你自己的等价实现）落进去 —— 目前挡在「这段看着像死代码，删掉吧」和那个守卫之间的，就只有这条评论。
- 在 PR 正文里点名两处真实修复：MCP 工具结果导致的传输被杀，以及这次的过期 load 结算。两者都是被归档为 refactor 的用户可见修复。
- 上一轮遗留仍然成立：
screenshots.spec.ts:778的歧义选择器会全仓性地压制视觉预览。 - 体量那点也仍然成立 —— 下一轮请分阶段落地。
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
|
Thank you for the thorough round-2 verification — added the R9-1 pinning fixture as provided in ea7e945. Verified locally at the new head: the fixture passes with the 中文说明感谢第二轮的详尽验证——已在 ea7e945 中按提供的原样加入了 R9-1 钉住测试。在新头上本地验证：守卫存在时测试通过；把守卫还原为仅按 session 身份认领后测试失败（ |
|
@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: 151 passed · 0 failed · 151 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：151 通过 · 0 失败 · 151 总计 抖动门： Verification reportPR 9682 verification (follow-up round) — refactor: deepen architecture ownership boundariesVerdict: 中文 — 判定：✅ 通过（agent 判定）本轮为跟进验证。上一轮唯一发现 F1（R9-1 守卫无测试钉住，突变存活）已被新提交
Previous-finding status
All previous measurements were re-run, not carried forward: the depth-2 checkout cannot reach Central claim + A/B (re-measured)Central claim: the transport-safety centralization is behavior-preserving — wire-legal Method: head arm = compiled
Census deltas vs the previous round's table are corpus-shape effects (27 vs 24 payloads; lenient Mutation matrix (all arms re-run at the new head)Witness:
Note: an earlier M3/M4 formulation was void (wrong whitespace / equivalent mutant) and is excluded F1 fix detailThe fixture drives exactly the interleaving the previous report said would pin the guard: two FindingsNo blocking findings. Non-blocking observations:
Not covered
MethodologyEnvironment: CI merge-ref checkout ( Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
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.
12 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R13-1 watchdog release-before-callback ordering unpinned (attachment-lifecycle.test.ts:96) — already reported as R12-2 and round-2/3/5/6/7/8 convergence-posture deferrals (reviews 5013080184, 5014440443, 5029145840, 5032096362, 5045295751, …
- admission-budget release on handler-exception path unpinned (transport-safety.test.ts:254) — already reported as round-6/7 convergence-posture deferrals (reviews 5032096362, 5045295751) and R8-9
- error-keyed releaseMessage branch unpinned (transport-safety.test.ts:394) — already reported as round-5/7 convergence-posture deferrals (reviews 5029145840, 5045295751) and R8-13
- object-identity-keyed release path unpinned (transport-safety.test.ts:394) — already reported as the round-9 deferral (review 5054370521, runRequest reservation identity)
- log-safe mask errorKind/status/hint preservation contract unpinned (transport-safety.ts:491) — already reported as the round-9 deferral (review 5054370521)
- phantom acp-session-bridge denylist entry (scripts/check-serve-fast-path-bundle.js:84) — already reported as the round-5 deferral (review 5029145840)
- estimator number arm and boolean-false unpinned (transport-safety.test.ts:50) — already reported as the round-7 deferral (review 5045295751) and R8-8
- record-cycle seen-check mutant survives (transport-safety.test.ts:152) — already reported as the round-3 deferral (review 5014440443) and R8-12
- throwing onTimeout / load-always-settles guarantee unpinned (attachment-lifecycle.ts:72) — already reported as the round-6 deferral (review 5032096362), R8-3 and R12-4
- retained-budget count arm unpinned (transport-safety.ts:86) — already reported as the round-12 deferral (review 5057370479)
- R9-1 provider-layer release-guard wiring unpinned (DaemonSessionProvider.tsx:1619) — already reported as the round-10 deferral (review 5055463393) and the R11-1/R12-1 lineage
- watchdog clearTimeout disarm unpinned (attachment-lifecycle.ts:106) — already reported as the round-8 deferral (review 5051873342)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 12 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — did not converge within the reverse-audit round cap of 3。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
Merge-conflict status: this branch currently conflicts with I attempted the merge and resolved the textual conflicts, but the standalone-session suite ( 中文说明合并冲突状态：本分支当前与 |
3 participants
What this PR does
This PR deepens ownership boundaries across five high-churn architecture seams while preserving existing public protocols and user-visible behavior. It centralizes bounded ACP transport safety, gives workspace-qualified routes one trusted-runtime selection policy, removes an obsolete CLI compatibility layer in favor of owning package boundaries, centralizes Web Shell prompt-admission and retry decisions, and gives WebUI session attachment a single lifecycle owner.
Why it's needed
These responsibilities were previously duplicated or spread across large composition roots, which made safety rules and concurrency invariants difficult to audit. The refactor improves locality, removes divergent implementations, and makes future changes less likely to bypass transport limits, workspace trust checks, stale retry ownership, or session-load generation checks.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — this is a behavior-preserving internal refactor with no intended UI changes.
Tested on
Environment (optional)
Node.js v24.18.0 on macOS. The full repository build, workspace typecheck, and repository lint passed. Focused validation passed for the ACP bridge (795 tests), affected workspace routes and CLI bridge wiring (219 tests), Web Shell admission/retry behavior (496 tests), and WebUI attachment/provider behavior (337 tests). The final diff passed whitespace validation and two clean self-audit passes.
Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
这个 PR 在保持现有公共协议和用户可见行为不变的前提下，深化了五个高频变更架构接缝的职责边界：集中管理有界 ACP 传输安全策略，为 workspace-qualified 路由提供统一的可信运行时选择策略，移除过时的 CLI 兼容层并直接依赖所属 package 边界，集中管理 Web Shell 的 prompt admission 与 retry 决策，并让 WebUI session attachment 拥有单一生命周期管理者。
为什么需要它
这些职责此前存在重复实现，或者散落在大型组合根中，导致安全规则和并发不变量难以审计。本次重构提升了职责局部性，删除了可能逐渐分叉的重复实现，并降低后续改动绕过传输限制、workspace trust 检查、过期 retry ownership 或 session load generation 校验的风险。
Reviewer 测试计划
如何验证
证据（Before & After）
N/A——这是保持行为不变的内部重构，没有预期 UI 变化。
测试平台
环境（可选）
在 macOS 和 Node.js v24.18.0 下完成验证。整仓构建、workspace typecheck 和 repository lint 均通过。聚焦验证包括 ACP bridge（795 个测试）、受影响的 workspace route 和 CLI bridge wiring（219 个测试）、Web Shell admission/retry 行为（496 个测试），以及 WebUI attachment/provider 行为（337 个测试）。最终 diff 通过 whitespace 校验和两轮干净的自审。
风险与范围
关联 Issue
N/A
Live bug fixes included in this refactor
Two of the ownership-boundary changes double as fixes for bugs present on
maintoday:main,pendingLoadToResolveis keyed on session identity alone, so two overlapping restores of the same session share the session object and a superseded run can resolve the successor's pending load (the newer session switch reports itself complete while its ownload/resumeis still in flight). TheattemptedLoadForIterationguard inDaemonSessionProvider.tsxprevents this; pinned by the R9-1 fixture inDaemonSessionProvider.test.tsx(does not let a superseded restore settle the successor pending load, ea7e945), which fails onmain's session-identity-only adoption.