fix(serve): Close daemon ACP resource guard gaps - #8947
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review convergence report for the #8911 follow-up:
Verification on the final main baseline:
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Re-run on the new head — gate conclusions unchanged. Template looks good ✓ Problem: unchanged from the first pass — planned work in the daemon resource-protection track (#8091 "Bound ACP and transport containers"), follow-up to the merged #8911. The gaps are structural properties of the ACP SDK connection model identified in that work, and the intervening Direction: aligned — implements a tracked #8091 delivery item while preserving standalone/public stream defaults, exactly as the tracking issue prescribes. Size: all production changes stay in Approach: scope matches the stated goal, no drive-by changes. The delta over the previously reviewed head is exactly what the review batch asked for — response exemption from the structural caps, bounded walker accounting, approval-mode compensation on rejected attach, incremental estimator — plus oracle tests for each. Risk: no production match against the revert-correlated high-risk path list (the Moving on to code review. 🔍 中文说明新 head 上的 re-run——门禁结论不变。 模板完整 ✓ 问题：与首轮一致——daemon 资源保护系列（#8091"Bound ACP and transport containers"）中的计划内工作，是已合并 #8911 的后续。这些缺口是该工作中确认的 ACP SDK 连接模型结构性问题，期间的 方向：对齐——实现 #8091 跟踪的交付项，并按跟踪 issue 的要求保留 standalone/public stream 的默认行为。 规模：所有生产改动都在 方案：范围与目标一致，无夹带改动。相对上一个被审查 head 的增量恰好是 review 批次要求的内容——response 豁免结构上限、walker 有界计数、attach 被拒时的 approval-mode 回补、增量估算器——并为每一项配了 oracle 测试。 风险：未命中 revert 相关的高风险生产路径（ 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwn-code /triage |
Code review (re-run on the new head)The previous round blocked on one CI failure and the
The surrounding machinery reads well on a second pass: Non-blocking, unchanged from last round: the Transport lifecycle (bounded daemon-owned channel)sequenceDiagram
participant P1 as ACP child
participant P2 as Bounded NDJSON reader
participant P3 as Envelope and schema validation
participant P4 as Admission ledgers
participant P5 as ACP SDK dispatch
participant P6 as Bridge generation
P1->>P2: frame bytes
P2->>P3: parse, structure and scalar checks
P3->>P4: admit request by count, bytes and id
P4->>P5: handler runs under cap
P5-->>P4: response settles, slot released
P3-->>P6: violation or saturation retires the exact generation
Testing evidence (PR's own CI, read via API — PR code never executed here)The unit suite — the signal this PR most needs — is green on the reviewed commit, first attempt, no flaky reruns: acp-bridge ran 26 files / 1,266 tests and the CLI package ran 798 files / 19,061 tests, including the two files that failed to collect last round. The macOS/Windows unit legs and the CLI integration job show as skipped; that is this repo's design, not a gap —
The sandboxed lane named last round is already in flight on this head — the Not verified: real backpressure on Windows/Linux (author tested on macOS only per the template — author's claim), and aggregate cross-workspace quotas (declared out of scope). 中文说明代码审查（新 head 上的 re-run）上一轮阻断于一项 CI 失败，
第二遍读下来周边机制同样站得住： 非阻断、与上轮一致： （传输生命周期时序图见英文部分。） 测试证据（读取 PR 自身 CI，未在此执行任何 PR 代码）单测套件——本 PR 最需要的信号——在被审查 commit 上一次通过即为全绿，无 flaky 重跑：acp-bridge 26 个文件 / 1,266 个测试，CLI 包 798 个文件 / 19,061 个测试，包括上一轮无法收集的两个文件。macOS/Windows 单测腿与 CLI 集成 job 显示为跳过；这是本仓库的设计而非缺口—— 上一轮点名的沙箱通道已在本 head 上运行—— 未验证：Windows/Linux 上的真实 backpressure（模板显示作者仅在 macOS 上测试——作者陈述）；跨 workspace 聚合配额（已声明范围外）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — everything that blocked last round is fixed and oracle-tested, CI is green first try, and a maintainer has already signed off after a deep read; the residual items are non-blocking nits, named below. Stepping back: this re-run is exactly the shape a fix round should have. The four items the review batch raised were not waved away — each maps to a code change I can point at and a test that would fail without it: the response exemption keeps owned bulk responses deliverable while requests stay structure-capped, the walker's push-time accounting bounds its own stack, the rejected-attach path now restores the previous approval mode before rolling back ownership, and the estimator stops walking once the budget is gone. My independent baseline for this problem (pre-dispatch admission ledger, mirrored outbound ledger, SDK-schema validation, exact-generation retirement, strict opt-in) matches what shipped; I did not find a simpler path that still covers the #8091 item. What keeps this at 4 rather than 5: the density of the ledger machinery is a real maintenance cost — it is mapped by comments and the design doc, but the next person to touch this state machine will need both; a batch of Suggestion-level threads from the latest automatic review round remains unresolved and is being deferred under the five-round rule (worth a follow-up issue so nothing is silently dropped); and the None of that blocks: no unresolved Critical, the only prior blocker is fixed, CI is green on the reviewed commit, and the maintainer's approval on this head came with a substantive read (including the SDK identity-coupling check). Approving, pinned to the reviewed commit. ✅ 中文说明置信度：4/5 —— 上一轮的所有阻断项均已修复并配有 oracle 测试，CI 首次运行即全绿，维护者也已在深入阅读后签字；剩余项均为非阻断的小问题，列在下方。 整体来看：这次 re-run 正是修复轮应有的样子。review 批次提出的四项问题都不是口头回应——每一项都能指到对应的代码改动，并有一个"缺了修复就会失败"的测试：response 豁免使自有 bulk response 可交付而 request 仍受结构上限约束；walker 的 push 时计数约束了自身栈；被拒 attach 路径在回滚所有权之前先恢复原 approval mode；估算器在预算耗尽时停止遍历。我对该问题的独立思路（分发前 admission 账本、镜像 outbound 账本、SDK schema 校验、准确 generation 回收、严格 opt-in）与最终实现一致；没有找到能覆盖 #8091 交付项的更简路径。 之所以是 4 而不是 5：账本机制的密度是真实的维护成本——有注释和设计文档作地图，但下一个改动该状态机的人两者都需要；最新一轮自动 review 留下一批未解决的 Suggestion 线程，正按五轮规则推迟（值得开个 follow-up issue，避免无声丢失）； 这些都不构成阻断：没有未解决的 Critical，唯一的先前阻断项已修复，被审查 commit 上 CI 为绿，维护者在本 head 上的批准附带了实质性阅读（包括 SDK 身份耦合的核对）。予以批准，钉在被审查 commit 上。✅ — 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.
Needs a small but mandatory fix before merge: the new top-level CLIENT_METHODS import in packages/acp-bridge/src/spawnChannel.ts breaks collection of packages/cli/src/acp-integration/acpAgent.test.ts and acpAgent.worktree.test.ts (their vi.mock('@agentclientprotocol/sdk') factories don't define it), so Test (ubuntu-latest, Node 22.x) fails on the reviewed commit and the macOS/Windows legs never ran. Add CLIENT_METHODS to both mocks (e.g. via importOriginal) or construct the validator map lazily. Details in my Stage 2 comment above — the rest of the work is solid. 🙏
合并前需要一个很小但必须的修复：packages/acp-bridge/src/spawnChannel.ts 新增的顶层 CLIENT_METHODS import 导致 packages/cli/src/acp-integration/acpAgent.test.ts 与 acpAgent.worktree.test.ts 无法收集（二者的 vi.mock('@agentclientprotocol/sdk') 工厂未定义该导出），被审查 commit 上 Test (ubuntu-latest, Node 22.x) 失败，macOS/Windows 腿未运行。请在两个 mock 中补上 CLIENT_METHODS（如用 importOriginal）或将 validator map 改为惰性构造。详见上方 Stage 2 评论——其余工作质量很高。
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the current review/CI blocker in e9cea55.
The branch has been pushed and CI is running on the new head. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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): You are review agent reverse-audit — Reverse audit agen...: none — all checks I started were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed; the hunk at diff line 2274 ( @@ -527,4 +850,84 @@ , same file's trailing tests) belongs to another agent's chunk and was de…; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; This PR hardens the daemon-owned ACP transport in package...: none — the walk above completed within budget., and 11 more.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks I started were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed; the hunk at diff line 2274 ( @@ -527,4 +850,84 @@ , same file's trailing tests) belongs to another agent's chunk and was de…；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；This PR hardens the daemon-owned ACP transport in package...：none — the walk above completed within budget.，另有 11 条。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Preserve owned bulk responses, stop bounded estimators once their limits are reached, and compensate approval-mode changes when an attach loses a teardown race. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Handled the latest review batch in
Verification: 631 focused ACP tests passed; |
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: 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): You are review agent reverse-audit — Reverse audit agen...: none — the final line 3716 of the diff file (one trailing context line) could not be re-read because the diff file was deleted from disk mid-review; it is hunk …; This PR hardens the daemon-owned ACP transport in package...: none — all checks above completed within budget.; chunk 4: couldn't execute tests (no node_modules in either worktree or parent checkout; monorepo install exceeds budget).; chunk 4: could not execute the new test — neither the review worktree nor the parent checkout has node_modules , and a monorepo install exceeded this chunk's budget; th…; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget., and 13 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
已审查。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — the final line 3716 of the diff file (one trailing context line) could not be re-read because the diff file was deleted from disk mid-review; it is hunk …；This PR hardens the daemon-owned ACP transport in package...：none — all checks above completed within budget.；chunk 4：couldn't execute tests (no node_modules in either worktree or parent checkout; monorepo install exceeds budget).；chunk 4：could not execute the new test — neither the review worktree nor the parent checkout has node_modules , and a monorepo install exceeded this chunk's budget; th…；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.，另有 13 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| function safeTransportFailureCode(error: unknown): string | undefined { | ||
| if (!isRecord(error)) return undefined; | ||
| const code = error['code']; |
There was a problem hiding this comment.
[Suggestion] safeTransportFailureCode uses /^[a-z0-9_.-]{1,64}$/iu — the i+u flag combination matches [a-z] via Unicode case folding, so U+017F (ſ) and U+212A (Kelvin sign K) pass the intended ASCII-only sanitizer and reach the transport_error_code telemetry attribute and the stderr breadcrumb verbatim. — Failure scenario: a transport-failure error whose code contains ſ or U+212A passes the sanitizer into telemetry/stderr (verified in Node: /^[a-z0-9_.-]{1,64}$/iu.test('ſ') === true). Latent today because producers are trusted lowercase-ASCII daemon constants, but the sanitizer's stated contract exists precisely to not depend on its producers; the i flag is gratuitous (no producer uses uppercase) while silently widening the admitted alphabet. Suggested fix: use the explicit ASCII class /^[a-z0-9_.-]{1,64}$/u (dropping i) — verified to reject both characters while still rejecting newlines.
中文说明
safeTransportFailureCode 使用 /^[a-z0-9_.-]{1,64}$/iu——i+u 标志组合会通过 Unicode 大小写折叠匹配 [a-z]，因此 U+017F（ſ）和 U+212A（开尔文符号 K）能通过本应仅限 ASCII 的净化器，原样进入 transport_error_code 遥测属性与 stderr 面包屑日志。— 失败场景：code 含 ſ 或 U+212A 的 transport-failure 错误会通过净化器进入遥测/stderr（已在 Node 中验证：/^[a-z0-9_.-]{1,64}$/iu.test('ſ') === true）。当前 producers 都是可信的小写 ASCII daemon 常量，故为潜在问题；但净化器的既定契约正是为了不依赖其 producers，且 i 标志是多余的（没有 producer 使用大写），却悄悄扩大了允许的字母表。修复建议：改用显式 ASCII 类 /^[a-z0-9_.-]{1,64}$/u（去掉 i）——已验证可拒绝上述两个字符，同时仍能拒绝换行符。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| info.isDying = true; | ||
| clearInFlightExtensionRefreshes(info.connection); |
There was a problem hiding this comment.
[Suggestion] The transport-failure rejection path for in-flight refreshExtensionsForAllSessions — this clearInFlightExtensionRefreshes(info.connection) call inside markTransportFailed — is exercised by no test. Probe: removing this call keeps the full acp-bridge suite green (1266/1266), while the control mutation (removing withTimeout from the refresh wait race) fails 12 tests, so the harness can see regressions in this mechanism but nothing pins this call. — Failure scenario: if this call regresses, a refresh awaiting a child response when the transport fails waits out the full 30 s withTimeout instead of failing promptly with BridgeChannelClosedError('refreshExtensionsForAllSessions'), delaying the {failed} result and the extensions_changed broadcast. Suggested fix: add a test that starts refreshExtensionsForAllSessions against a gated workspaceExtensionsRefresh ext method, resolves the channel's transportFailed, and asserts the refresh settles promptly as failed rather than after the 30 s budget.
中文说明
针对进行中 refreshExtensionsForAllSessions 的 transport-failure 拒绝路径——即 markTransportFailed 内的这处 clearInFlightExtensionRefreshes(info.connection) 调用——没有任何测试覆盖。探针验证：删除该调用后完整 acp-bridge 套件仍全绿（1266/1266），而对照突变（移除 refresh wait 竞态中的 withTimeout）会使 12 个测试失败，说明测试框架能够发现该机制的回归，但没有任何测试钉住这处调用。— 失败场景：若该调用回归性地被移除，transport 失败时正在等待 child 响应的 refresh 会等满 30 秒 withTimeout，而不是立即以 BridgeChannelClosedError('refreshExtensionsForAllSessions') 失败，从而延迟 {failed} 结果与 extensions_changed 广播。修复建议：新增测试——以受控的 workspaceExtensionsRefresh ext method 启动 refreshExtensionsForAllSessions，然后 resolve channel 的 transportFailed，断言 refresh 迅速以失败结束，而非等满 30 秒预算。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await bridge.shutdown(); | ||
| }); | ||
|
|
||
| it('restores approval mode when an attach loses a close race', async () => { |
There was a problem hiding this comment.
[Suggestion] The new apply→re-assert→rollback pairing added to restoreSession's live-session branch (bridge.ts:5589-5605), the coalescer branch (~5709-5724), and the racedEntry branch (~6140-6156) has no gating test; this close-race test gates only the spawnOrAttach existing-entry sibling of the same pattern. — Failure scenario: if a live-branch rollback were later deleted or reordered, a loadSession/resumeSession with approvalMode that loses the conditional-close race (close authorized, then refused with held work) would leave the surviving session promoted to the rejected attacher's mode, and the suite stays green — the exact regression this new test was written to catch, on the sibling branch. Suggested fix: mirror this test driving bridge.loadSession({ sessionId: <live id>, approvalMode, clientId }) with the conditional-close race, asserting modeCalls = [YOLO, DEFAULT] and session survival.
中文说明
新增于 restoreSession 存活 session 分支（bridge.ts:5589-5605）、coalescer 分支（~5709-5724）与 racedEntry 分支（~6140-6156）的 apply→re-assert→rollback 配对没有任何门控测试；本 close-race 测试只门控了同一模式在 spawnOrAttach 已有 entry 上的兄弟分支。— 失败场景：若某个 live 分支的 rollback 日后被删除或改序，带 approvalMode 的 loadSession/resumeSession 在条件关闭竞争中落败（关闭已获授权，随后因 child 持有工作被拒绝）时，存活的 session 会保留被拒绝 attach 方的 mode，而套件仍然全绿——这正是本新测试意在捕获的回归，只是发生在兄弟分支上。修复建议：仿照本测试，用条件关闭竞争驱动 bridge.loadSession({ sessionId: <存活 id>, approvalMode, clientId })，断言 modeCalls = [YOLO, DEFAULT] 且 session 存活。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| newSessionImpl: async () => { | ||
| failure.resolve(transportError); | ||
| await Promise.resolve(); | ||
| return { sessionId: 'response-before-fatal' }; | ||
| }, |
There was a problem hiding this comment.
[Suggestion] The two new handshake-failure tests ('does not publish a channel whose transport fails during initialize' and 'does not register a session whose transport fails with newSession') resolve transportFailed before the impl returns, so they settle through the also-new channelUnavailableReject race competitor and never execute the new post-handshake isDying guards (bridge.ts:3569-3572 and 3742-3747). — Failure scenario: probe-verified — removing BOTH guards keeps these tests green and the full bridge suite 557/557 green. A future change deleting the guards — or the await Promise.resolve() yield the newSession guard documents as load-bearing — ships green, and a response admitted immediately ahead of a fatal frame installs/publishes a session on a dying channel. Suggested fix: make each layer individually observable — in one variant resolve the failure AFTER the response has been admitted (or drop the transportFailed override and set isDying via the settled-failure path) so only the guard can reject, and assert the guard-specific error message ('after newSession' / 'during initialize').
中文说明
两个新的握手失败测试（'does not publish a channel whose transport fails during initialize' 与 'does not register a session whose transport fails with newSession'）在 impl 返回之前就 resolve 了 transportFailed，因此它们经由同样是新增的 channelUnavailableReject 竞争方结束，从未执行新的握手后 isDying 守卫（bridge.ts:3569-3572 与 3742-3747）。— 失败场景：探针验证——同时移除两个守卫后这些测试仍为绿，完整 bridge 套件 557/557 全绿。未来若删除守卫——或删除 newSession 守卫注释中说明为关键的 await Promise.resolve() 让位——会在测试全绿的情况下合入，导致紧邻 fatal frame 之前被接纳的响应在垂死 channel 上安装/发布 session。修复建议：让每一层可独立观测——在某个变体中于响应被接纳之后再 resolve 失败（或去掉 transportFailed 覆盖、改由已决失败路径设置 isDying），使只有守卫能触发拒绝，并断言守卫特有的错误信息（'after newSession' / 'during initialize'）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| wait: Promise.race([ | ||
| withTimeout( | ||
| promise, | ||
| 30_000, | ||
| SERVE_CONTROL_EXT_METHODS.workspaceExtensionsRefresh, | ||
| ), | ||
| unavailable, | ||
| ]), |
There was a problem hiding this comment.
[Suggestion] When a live in-flight extension-refresh record is REPLACED in inFlightExtensionRefreshes (bootstrap escalation via refreshBootstrap && !inFlight.refreshBootstrap, or a connection change), the displaced record's rejectUnavailable becomes unreachable — clearInFlightExtensionRefreshes walks only current map slots — so displaced waiters lose the fast transport-failure rejection and sit until the 30 s withTimeout. — Failure scenario: verified real overlap path — workspace-extensions-controller.ts races the bridge call against EXTENSION_REFRESH_TIMEOUT_MS and on timeout releases the commit lane without cancelling the in-flight bridge call; reconciliation then calls bridge.refreshExtensionsForAllSessions directly. If that second call replaces the first call's record and the old record's transport then fails, the first call's waiter is never rejected via unavailable and its result lands up to ~30 s after the channel is already known dead. Pre-PR every awaiter raced getTransportClosedReject(entry) itself and failed immediately. Suggested fix: keep displaced records reachable for transport-failure rejection — e.g. push the outgoing record into a side list that clearInFlightExtensionRefreshes also drains, or reject the outgoing record's wait at replacement time.
中文说明
当 inFlightExtensionRefreshes 中一个存活的进行中 extension-refresh 记录被替换（经 refreshBootstrap && !inFlight.refreshBootstrap 的 bootstrap 升级，或 connection 变更）时，被置换记录的 rejectUnavailable 变得不可达——clearInFlightExtensionRefreshes 只遍历当前 map 槽位——被置换的等待方失去快速的 transport-failure 拒绝，只能等满 30 秒 withTimeout。— 失败场景：已验证的真实重叠路径——workspace-extensions-controller.ts 将 bridge 调用与 EXTENSION_REFRESH_TIMEOUT_MS 竞态，超时后释放 commit 通道但不取消进行中的 bridge 调用；随后 reconciliation 会直接调用 bridge.refreshExtensionsForAllSessions。若第二个调用替换了第一个调用的记录，而旧记录的 transport 随后失败，第一个调用的等待方永远不会被 unavailable 拒绝，其结果会在 channel 已被确认死亡后最多约 30 秒才落地。PR 之前每个等待方各自 race getTransportClosedReject(entry)，能立即失败。修复建议：让被置换记录仍可被 transport-failure 拒绝触达——例如把被换出的记录压入一个 side list，让 clearInFlightExtensionRefreshes 一并清空；或在替换时直接 reject 被换出记录的 wait。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await expect(attach).rejects.toMatchObject({ code: 'session_closing' }); | ||
| expect(modeCalls).toEqual([ApprovalMode.YOLO, ApprovalMode.DEFAULT]); |
There was a problem hiding this comment.
[Suggestion] This test gates only the approval-mode half of the rejected-attach rollback; the attach-bookkeeping half (rollbackAttachRegistration in the same re-assert catch) is asserted nowhere for this path. — Failure scenario: probe-verified mutants — removing rollbackAttachRegistration from the exercised single-scope re-assert catch (bridge.ts:6840) or from the restore sibling (bridge.ts:5603) keeps this test green and the full bridge suite 557/557 green. Harm probe observed the consequence: the surviving session carries a phantom registered client (knownClientIds count 2 vs correct count 1), leaking a client whose attach was refused and skewing zero-attach-based reaping / requireZeroAttaches teardown decisions; the probe flips on the correct code. Suggested fix: attach with an explicit clientId and, after the session_closing rejection, assert expect(bridge.knownClientIds()).not.toContain('<that clientId>') before resolving closeResult.
中文说明
本测试只门控了被拒绝 attach 回滚的 approval-mode 一半；attach 簿记一半（同一 re-assert catch 中的 rollbackAttachRegistration）在该路径上没有任何断言。— 失败场景：探针验证的突变——从被执行的 single-scope re-assert catch（bridge.ts:6840）或 restore 兄弟位置（bridge.ts:5603）移除 rollbackAttachRegistration 后，本测试仍为绿，完整 bridge 套件 557/557 全绿。危害探针观察到后果：存活的 session 携带一个幽灵注册 client（knownClientIds 计数 2 而非正确的 1），泄漏了一个 attach 已被拒绝的 client，并干扰基于零 attach 的回收 / requireZeroAttaches 拆除决策；探针在正确代码上翻转。修复建议：使用显式 clientId 发起 attach，在 session_closing 拒绝之后、resolve closeResult 之前断言 expect(bridge.knownClientIds()).not.toContain('<该 clientId>')。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| 'qwen-code.daemon.channel.transport_error_code': | ||
| 'ndjson_frame_too_large', |
There was a problem hiding this comment.
[Suggestion] In 'transport failure marks the channel dying before process exit', the exit-telemetry assertions conflate the two crashed channels: this objectContaining matcher pins transport_error_code: 'ndjson_frame_too_large', which channel 2's event (ndjson_queue_limit_exceeded, restore-time teardown on a fresh channel — the only test exercising that exit shape) can never match, and both channels emit identical channelLifecycle('exit', false) calls, so the matchers are fully satisfied by channel 1 alone. — Failure scenario: probe-verified — adding an assertion pinning channel 2's shape passes in this very test, so the event is emitted but asserted nowhere in the suite (grep found only this site and the planned-teardown shape at :8388). A future refactor special-casing the restore-time teardown's telemetry ships green, silently losing exit observability for exactly the failure class this PR adds. Suggested fix: pin channel 2's exit separately — add expect(event).toHaveBeenCalledWith('channel.exited', expect.objectContaining({ 'qwen-code.daemon.channel.transport_error_code': 'ndjson_queue_limit_exceeded' })) to the waitFor.
中文说明
在 'transport failure marks the channel dying before process exit' 中，exit 遥测断言把两个崩溃的 channel 混在一起：此 objectContaining 匹配器钉住了 transport_error_code: 'ndjson_frame_too_large'，而 channel 2 的事件（ndjson_queue_limit_exceeded，新 channel 上 restore 期间的拆除——唯一覆盖该 exit 形态的测试）永远无法匹配；两个 channel 又发出完全相同的 channelLifecycle('exit', false) 调用，因此这些匹配器仅凭 channel 1 即可完全满足。— 失败场景：探针验证——在本测试中补充钉住 channel 2 形态的断言即可通过，说明该事件确实被发出，但全套件中没有任何断言覆盖它（grep 仅找到本处与 :8388 的计划拆除形态）。未来若某个重构对 restore 期间拆除的遥测做特殊化处理，会在测试全绿的情况下合入，悄悄丢失本 PR 新增的这一失败类别的 exit 可观测性。修复建议：单独钉住 channel 2 的 exit——在 waitFor 中补充 expect(event).toHaveBeenCalledWith('channel.exited', expect.objectContaining({ 'qwen-code.daemon.channel.transport_error_code': 'ndjson_queue_limit_exceeded' }))。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await bridge.shutdown(); | ||
| }); | ||
|
|
||
| it('releases branch admission when transport fails during restore cleanup', async () => { |
There was a problem hiding this comment.
[Suggestion] This test can never execute the branchSession restore-cleanup block the PR changed: markTransportFailed (registered at channel attach) sets ci.isDying = true deterministically before branchSession's catch runs, so the new if (!ci.isDying) race/timeout block is always skipped — the test exercises only the skip. — Failure scenario: probe-verified — deleting the entire guarded cleanup block keeps this test green (the releases === ['branch'] assertion comes from the outer finally; the no-sessionClose assertion then holds trivially). An instrumented run of all 557 bridge tests found exactly one catch entry with isDying=true and zero with isDying=false: the cleanup-attempt path (restore fails on a still-live channel, e.g. agent resource-not-found) has no coverage anywhere. A revert reintroducing the indefinite sessionClose await on the error path ships green. Suggested fix: add a sibling test where the branch's loadSessionImpl throws a non-transport error (channel stays healthy, isDying false) and assert sessionClose is then called with the branched session id.
中文说明
本测试永远无法执行 PR 修改的 branchSession restore-cleanup 代码块：markTransportFailed（在 channel attach 时注册）会在 branchSession 的 catch 运行之前确定性地设置 ci.isDying = true，因此新增的 if (!ci.isDying) race/timeout 块总是被跳过——测试只覆盖了跳过分支。— 失败场景：探针验证——整体删除受保护的 cleanup 块后本测试仍为绿（releases === ['branch'] 断言来自外层 finally；无 sessionClose 的断言随之平凡成立）。对全部 557 个 bridge 测试的插桩运行只发现一次 isDying=true 的 catch 进入、零次 isDying=false：cleanup 尝试路径（restore 在仍存活的 channel 上失败，如 agent resource-not-found）没有任何覆盖。若回归性地在错误路径上重新引入无限期的 sessionClose await，会在测试全绿时合入。修复建议：新增兄弟测试，让分支的 loadSessionImpl 抛出非 transport 错误（channel 保持健康、isDying 为 false），断言随后会以分支出的 session id 调用 sessionClose。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const releaseOutbound = channel.transportGuard?.reserveOutboundOperation([ | ||
| 'method', | ||
| { optional: undefined }, | ||
| ]); | ||
| releaseOutbound?.(); |
There was a problem hiding this comment.
[Suggestion] The release side of both spawnChannel transport-guard budgets is ungated: this releaser call asserts nothing about the release effect (the releaser is the ONLY code that decrements OutboundOperationBudget counters — bridge.test.ts mocks the guard as reserveOutboundOperation: () => () => {}), and no test writes an error response through a real stream, so PreparedResponseBudget.releaseMessage's message['error'] branch (paired with the result.error reservation at bridge.ts:303) never executes. — Failure scenario: probe-verified mutants — 'releaser never decrements' and 'releaseMessage error branch removed' both keep the full acp-bridge suite green (1266/1266) and run-qwen-serve green (243/243); the probes flip red with count exhaustion (NDJSON decoded queue is full, ample bytes remaining). Production consumes one outbound reservation per daemon round-trip via createOutboundGuardedConnection, so a broken releaser retires a healthy long-lived channel generation after 256 cumulative operations. Distinct from the deferred round-1 ledger-release threads (ndJsonStream's ledgers — different family/file). Suggested fix: assert the release effect here (reserve A, release A, then reserve B and C and assert the third throws), and add a prepared-error case: reserve an error object, write { jsonrpc: '2.0', id, error } through a real stream, then assert a subsequent reservation succeeds.
中文说明
spawnChannel 两个 transport-guard 预算的释放侧均无门控：此处 releaser 调用对释放效果没有任何断言（该 releaser 是唯一递减 OutboundOperationBudget 计数的代码——bridge.test.ts 将 guard mock 为 reserveOutboundOperation: () => () => {}），且没有任何测试通过真实 stream 写出 error 响应，因此 PreparedResponseBudget.releaseMessage 的 message['error'] 分支（与 bridge.ts:303 处 result.error 预留配对）从不执行。— 失败场景：探针验证的突变——'releaser 从不递减' 与 '移除 releaseMessage error 分支' 都使完整 acp-bridge 套件保持全绿（1266/1266）、run-qwen-serve 保持全绿（243/243）；探针则以计数耗尽翻红（NDJSON decoded queue is full，字节余量充足）。生产环境经 createOutboundGuardedConnection 每次 daemon 往返消耗一个出站预留，因此损坏的 releaser 会在累计 256 次操作后回收一个健康的长寿命 channel generation。与已推迟的第一轮 ledger 释放线程不同（那是 ndJsonStream 的 ledger——不同家族/文件）。修复建议：在此断言释放效果（预留 A、释放 A，再预留 B 与 C 并断言第三次才抛错），并补充 prepared-error 用例：预留一个 error 对象，通过真实 stream 写出 { jsonrpc: '2.0', id, error }，然后断言后续预留成功。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } else if (typeof current === 'string') { | ||
| bytes += Buffer.byteLength(current) + 2; | ||
| } else if (typeof current === 'number') { |
There was a problem hiding this comment.
[Suggestion] estimatePreparedResponseBytes charges strings at raw UTF-8 byte length + 2 quotes (and record keys at byteLength + 3), ignoring JSON escaping, yet the estimator is the ceiling the prepared-response and outbound-operation budgets enforce against maxQueuedBytes (64 MiB in the daemon policy). — Failure scenario: probe measured on the real budget — reserving { content: '\u0001'.repeat(2900) } values (17,414 serialized bytes each, 4,962 estimated) admitted 3.48× the configured ceiling in retained wire bytes; the per-string worst case is ~6× (JSON.stringify expands each control char to a 6-byte \uXXXX). Concrete carrier: a child writes a control-character file via fs_write_text_file, then answers concurrent fs_read_text_file requests while stalling its stdin. The design doc bounds 'conservatively estimated retained payloads' until local pipe delivery — the release happens only after the encoded frame is written, so under-counting serialization expansion violates the bound's own wording. Suggested fix: charge a worst-case serialization bound for strings (e.g. +5 bytes per char < 0x20 and +1 per "/\), for both values and record keys — the probe with this fix flipped admission back inside the ceiling (ratio 0.7).
中文说明
estimatePreparedResponseBytes 对字符串按原始 UTF-8 字节长度 + 2 个引号计费（record key 按 byteLength + 3），忽略了 JSON 转义，而该估算值正是 prepared-response 与 outbound-operation 预算针对 maxQueuedBytes（daemon 策略为 64 MiB）强制执行的上限。— 失败场景：探针在真实预算上测量——预留 { content: '\u0001'.repeat(2900) } 值（每个序列化后 17,414 字节、估算仅 4,962）时，实际接纳的保留线路字节达到配置上限的 3.48 倍；单字符串最坏情况约 6 倍（JSON.stringify 将每个控制字符展开为 6 字节 \uXXXX）。具体载体：child 先经 fs_write_text_file 写入一个控制字符文件，然后在停滞 stdin 的同时应答并发的 fs_read_text_file 请求。设计文档约束的是'保守估算的保留载荷'直至本地管道投递——而释放只在编码后的 frame 写出之后发生，因此低估序列化膨胀违反了该约束自身的措辞。修复建议：对字符串按最坏序列化界计费（如每个 < 0x20 的字符 +5 字节、每个 "/\ +1 字节），值与 record key 均适用——应用该修复的探针使接纳量回落到上限之内（比率 0.7）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed against head e2c6af2. Approving — the resource guards are real, not cosmetic, and the gap cases have direct test coverage.
What was verified:
- Transport-failure-before-exit gap:
channel.transportFailedresolves before process exit on any bound violation or clean EOF; the bridge flipsisDying, rejects in-flight extension refreshes, and races initialize/newSession/prompt/status/cd/branch/restore paths against it viachannelUnavailableReject, falling back toexitedwhen providers don't expose the signal. A child that closes stdout but stays alive can no longer be admitted to or reused (covered by "terminates a bounded child that closes stdout without exiting"). - SDK retention gaps: the ACP SDK dispatches handlers without awaiting and keeps its own write queue; the new inbound-handler admission (256/64MiB), prepared-response budget (held until the frame is actually written), outbound-operation reservation, and outbound-request/response id correlation close that. I checked the identity coupling against SDK 0.14.1:
#tryCallRequestHandlerreturns{result: result ?? null}, which matches the bridge'sreservePreparedResponse(result ?? null)exactly, so WeakMap/primitive release lookups hit for both object and undefined/null results. Crossing any bound retires exactly that channel generation (failTransportis single-shot, destroys both pipes, closes budgets). - Expected-exit accounting:
transportFailureInitiatedTeardownis only set when!isDying, so a planned teardown that gets a secondary pipe failure still reports as expected ("keeps a planned teardown expected when the pipe fails secondarily"). - Attach race consistency:
assertAttachableSessionEntryunifies the admission predicate and is re-checked after each async step; losing a close race now rolls back both the approval-mode change and the client registration ("restores approval mode when an attach loses a close race"). - Log safety: bounded messages install non-enumerable
inspect.customredaction, child-controlled error payloads are sanitized to bounded code/message/data before SDK logging, and telemetry only retains a regex-validated failure code. - Backward compat: guard/signal are optional on
AcpChannel,fatalCleanEofdefaults to false, and unbounded/public bridges keep legacy behavior (covered by test).
CI: all check runs green on head sha.
Nits (non-blocking, P3):
estimateAcpHandlerBytesin bridge.ts doesn't guard getters/toJSONthe wayestimatePreparedResponseBytesdoes; safe today because params come fromJSON.parse, but a short comment noting that assumption would help future readers.sessionCdstill relies solely on the transport-closed race without awithTimeout(pre-existing; other one-shot ext paths got timeouts in this PR — could align later).
|
@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: 49 passed · 1 failed · 50 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：49 通过 · 1 失败 · 50 总计 Verification reportVerification report — PR #8947
|
| cell | head | base | flip? |
|---|---|---|---|
| unknown response id | fatal ndjson_invalid_message, 0 delivered |
enqueued, no error | yes |
| clean EOF (bounded) | fatal ndjson_unexpected_eof, admitted frame still delivered |
graceful close | yes |
| non-JSON-RPC envelope | fatal ndjson_invalid_message |
enqueued | yes |
| parse error | fatal + metadata-only (sha256, payload omitted) | non-fatal + metadata-only | fatality yes |
| inbound req saturation (300, never answered) | 256 admitted then fatal queue_limit |
300 admitted | yes |
| outbound req saturation (300, no response) | 256 writes then refuse + transport error | 300 ok | yes |
| oversized method(1025B)/id(257B)/depth(65)/array(4097) | fatal invalid | enqueued | yes |
| at-limit method(1024B)/id(256B)/depth(64)/array(4096) | accepted | accepted | A/A |
| result+error both | fatal invalid | enqueued | yes |
| incomplete frame at EOF | fatal incomplete | fatal incomplete | A/A |
| matching response races write callback | accepted | accepted | A/A |
| legacy unbounded path | payload logged, continues | identical | A/A |
Spawn-level A/B with a real child (harness/spawn-ab.mjs + fake-child.mjs, production DAEMON_ACP_NDJSON_LIMITS): head 3/3, base 4/4 (03-spawn-ab-head.png, 04-spawn-ab-base.png).
| scenario | head | base |
|---|---|---|
| child closes stdout, stays alive | transportFailed=ndjson_unexpected_eof BEFORE exited, child terminated |
no failure signal; child still alive after 2.5s (dead-but-owned) |
| unknown response id | ndjson_invalid_message, child terminated |
frame delivered, child alive |
| known-method schema violation | ndjson_invalid_message, sha256-only log, no payload leak |
frame reaches SDK dispatch |
Findings
F1 — new test fails deterministically as committed (gate red)
bridge.test.ts transport failure marks the channel dying before process exit fails 6/6 (isolated ×3, full-file, full-suite). Its loadSession({…, historyPageSize: 10}) omits historyReplay: 'response'; the pre-existing (unchanged) gate at bridge.ts:5571-73 only honors historyPageSize when historyReplay==='response' (default 'stream'), so the transcript extMethod is never invoked and the first waitFor (expecting sessionTranscript in extMethodCalls) times out. Production behavior is correct: adding historyReplay:'response' makes the test pass and the whole file 568/568. One-line test fix. Repro: cd packages/acp-bridge && npx vitest run src/bridge.test.ts -t "transport failure marks the channel dying". Capture 07-new-test-fails-unpatched.png. Workspace gate: acp-bridge 1 failed / 1276 passed (1277); the failing test is this one.
F2 — real outbound-operation budget cap unpinned (coverage gap, not defect)
Mutation M5 (disable the throw in OutboundOperationBudget.reserve, spawnChannel.ts) survived: no repo test exercises the real cap — bridge.test.ts stubs transportGuard.reserveOutboundOperation, and spawnChannel.test.ts only calls it once then releases. Probe (harness/outbound-budget-probe.mjs, 05-outbound-budget-probe.png) confirms the behavior works: 3rd reserve under maxQueuedMessages=2 throws ndjson_queue_limit_exceeded, transportFailed fires, child terminated, release frees capacity. So a coverage gap; the guard is live and correct. Severity low/medium.
Mutation matrix (vacuity)
Positive control M0 (frame-size assert disabled) killed 5 tests incl. rejects an oversized inbound frame… — proves the harness can go red and the failure names expected-vs-actual.
| mutation | guard removed | result |
|---|---|---|
| M0 (positive control) | frame-size assert | killed (5) |
| M1 | fatal clean EOF | killed exactly 1 (terminates a bounded child that closes stdout without exiting) — 06-mutation-m1-clean-eof.png |
| M2 | unknown-response-id accept | killed 2 (malformed-envelope + log-amplifying corpora both contain unknown-id lines) |
| M3 | inbound request ledger | killed exactly 1 (bounds requests retained by the ACP SDK while responses are blocked) |
| M4 | handler count/byte cap | killed exactly 2 (the two handler-cap tests) |
| M5 | outbound budget cap | survived → coverage gap (F2) |
5/6 killed, 1 survivor classified coverage-gap (not dead code).
Not covered
- End-to-end real-SDK backpressure for the bridge-level handler admission (verified via mutation M4 + repo tests, not a live blocked-writable probe).
- Aggregate cross-workspace quotas, pre-allocation canonical encoder, EventBus/replay bounds, Windows/Linux process-tree behavior (PR-declared out of scope).
- Per-commit attribution (shallow depth-2 checkout; verified aggregate
HEAD^1..HEAD). - Did not re-run repo-wide lint/typecheck (PR's own CI covers; targeted gates run instead).
Methodology
Environment: node:22-bookworm CI container, merge-ref checkout (HEAD=merge, HEAD^1=base, HEAD^2=head). Head build pre-built; base control compiled only the changed modules into a copy of head dist/. Harnesses are mock-free w.r.t. the unit under test (real streams, real child processes, compiled dist/), driven via QWEN_CLI_ENTRY. Raw logs in logs/, harnesses in harness/, captures in evidence/. Assertions counted only from executed scripted checks.
Evidence images
Harness 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.
LGTM, looks ready to ship. ✅
|
Current I reproduced it twice in a clean detached The minimal fix is to add |
…8905) The merge of main pulled in #8933, which gates historyPageSize on historyReplay === 'response'. The 'transport failure marks the channel dying before process exit' test (from #8947) passes historyPageSize with the default stream replay, so the paged transcript fetch it waits on is never issued and the test times out — a cross-PR interaction between two main commits, failing deterministically on main. Pin the response replay mode the paged fetch requires.
…re test (QwenLM#8984) 'transport failure marks the channel dying before process exit' (added in QwenLM#8947) fails deterministically on merged main: it calls loadSession with historyPageSize but without historyReplay, and since QwenLM#8933 historyPageSize only takes effect when historyReplay is 'response' — the request silently degrades to stream replay, no sessionTranscript page is ever requested, and the extMethodCalls vi.waitFor times out. This was a logical merge conflict, not a flake: QwenLM#8947 branched before QwenLM#8933 landed, so both PRs were green on their own branches and red only once combined. Passing historyReplay: 'response' restores the intended path — the transcript page request hangs, the transport failure rejects it, and the second assertAttachableSessionEntry maps the torn-down session to SessionNotFoundError exactly as the test asserts. Co-authored-by: verify <verify@local>
|
Released in v0.21.11. |
…turn replay (QwenLM#8905) * feat(serve): adaptively grow live-journal caps before truncating mid-turn replay A single turn fanning out many concurrent subagents (e.g. a /review run) can emit hundreds of thousands of source events, far past the per-session live-journal baseline caps (10 000 entries / 8 MiB), so a mid-turn (re)load silently shows a truncated replay until the turn finishes. Before evicting, the engine now asks a growth advisor: caps double (entries scaled proportionally) while the growth granted across the bridge's live sessions fits in a pool derived from the daemon memory budget (5%, clamped to [32, 1024] MB), never past a per-session hard cap of 256 MiB. Growth is on demand, throttled after a refusal, and accounted statelessly from the current caps of all live sessions, so granted headroom dies with its session. An operator-pinned --max-journal-events/--max-journal-bytes disables growth; without a pool the fixed-cap eviction behavior is unchanged. * fix(serve): address adaptive live-journal growth review feedback (QwenLM#8905) * fix(serve): account in-flight restores in the journal growth pool (QwenLM#8905) Concurrent restores hold their buses in pendingRestoreEvents rather than byId, so each advisor ask only saw its own caps and concurrent restores could each draw a full doubling from the same pool. Sum the current caps of every in-flight restore bus into allSessionLimitBytes. Also skip the growth ask when the breaching append is a turn boundary — compactCurrentTurn discards the journal immediately afterwards, so the grant would be charged to the pool while buying zero eviction. Pin the previously untested contracts with tests: restore-window accounting, concurrent-restore accounting, headroom release on session close, the hard-cap clamp term, partial-grant eviction, requester discrimination in the policy fixtures, the maxEvents safe-integer conjunct, and the dynamic-workspace bridge pool wiring. Fix the docs: add the missing journal-flag rows to the daemon configuration and operations pages, and correct the effective-budget definition. * test(serve): request 'response' replay in the transport-failure test (QwenLM#8905) The merge of main pulled in QwenLM#8933, which gates historyPageSize on historyReplay === 'response'. The 'transport failure marks the channel dying before process exit' test (from QwenLM#8947) passes historyPageSize with the default stream replay, so the paged transcript fetch it waits on is never issued and the test times out — a cross-PR interaction between two main commits, failing deterministically on main. Pin the response replay mode the paged fetch requires. * fix(serve): share one daemon-wide journal growth pool (QwenLM#8905) Address the automated review of adaptive live-journal growth: - The growth pool is now one daemon-wide aggregate shared by every workspace bridge instead of a full pool per bridge, and growth is disabled when the budget is insufficient or leaves no headroom after the root reserve. - Grants that cannot retain any additional journal entries (an oversized event survives as the sole entry either way) are refused so the pool is never charged for growth that preserves no replay. - The refusal throttle defaults to a monotonic clock and treats a backward clock jump as an elapsed window. - The proportional event hard cap is clamped to MAX_SAFE_INTEGER so a valid-but-extreme baseline cannot poison every grant. - /daemon/status reports the growth semantics: limits.memory.journalGrowth (pool size, hard cap, baselines), per-session effective caps in full diagnostics, and enforced:false scoped to the child-heap model. - Validation-boundary tests for the growth-pool normalizer and doc fixes (positive safe integer types; growth toward double, limited by pool headroom). * fix(serve): align growth-pool docs and harden growth tests (QwenLM#8905) * fix(serve): account growth per session baseline and walk intermediate grants (QwenLM#8905) * fix(serve): harden growth-pool tests and derive help figures from constants (QwenLM#8905) * fix(serve): reject valueless journal cap flags and harden growth tests (QwenLM#8905) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
4 participants
What this PR does
This follow-up to #8911 closes resource-ownership gaps around the bounded daemon ACP transport. Daemon-owned channels now validate bounded JSON-RPC envelopes before ACP SDK dispatch; cap active handlers, prepared responses, pre-SDK outbound operations, and outstanding request IDs by both count and conservative byte charge; and retire the exact workspace channel generation when a protocol, serialization, EOF, or admission failure makes the transport unusable.
The bridge marks a failed channel unavailable before its child finishes the termination grace period, prevents initialize, create, restore, attach, prompt, and status work from reusing that generation, and rolls back provisional session ownership when delivery fails. Complete frames admitted before a fatal frame remain ordered and deliverable. Bounded messages and client errors use metadata-only inspection so the ACP SDK cannot independently log child-controlled payloads.
Public and standalone ACP streams remain opt-in: callers that do not supply limits or a transport guard keep the legacy queue, EOF, parse-log, and error-wire behavior.
Why it's needed
#8911 bounded raw NDJSON assembly and the decoded
ReadableStream, but the ACP SDK immediately drains that readable without awaiting request handlers and maintains private write and pending-response queues. A child could therefore bypass the stream watermark with slow handlers, stop reading stdin while continuing to send requests, return large responses to small requests, or never answer repeated daemon requests. Transport failure could also close the SDK connection before the child exited while the bridge still considered that generation reusable.These paths retained payloads, promises, sessions, and child processes outside the original queue accounting. Extending ownership through handler completion, response delivery, outbound request settlement, and exact-generation teardown keeps one abnormal workspace from growing the daemon indefinitely or leaving other requests stuck on a dead connection.
Reviewer Test Plan
How to verify
Use a real ACP SDK connection with a blocked writable and send more than 256 child-to-daemon requests whose handlers return responses. Confirm the first 256 remain bounded and the next admission retires only that channel. Repeat with daemon-to-child requests and notifications while the child does not read stdin; confirm admission occurs before the SDK private write queue and releases only at local notification delivery or request settlement.
Send malformed known-method parameters, oversized protocol scalars, unknown response IDs, incomplete EOF, and clean stdout EOF from a child that stays alive. Confirm each daemon-owned transport reports one typed, metadata-only failure, marks the exact channel generation unavailable immediately, terminates its tracked child, and leaves another workspace responsive. Confirm a valid matching response may arrive before the parent write callback without being rejected.
Exercise initialize, fresh session creation, existing attach, load/resume, branch cleanup, cwd change, shell-history injection, and extension refresh while transport failure races the operation. Confirm no provisional attach/session ownership, admission lease, SDK queue entry, or long-lived refresh reaction remains after teardown.
Finally, construct public streams and bridges without limits or a transport guard and confirm their legacy EOF, parse logging, and error response shape remain unchanged.
Evidence (Before & After)
N/A — non-UI daemon transport and lifecycle hardening.
Tested on
Environment (optional)
macOS arm64, Node.js 24.12.0, npm 10.9.8. Verified with 727 focused ACP bridge tests, 243 daemon runtime tests, full build, full typecheck, full lint, Prettier check, diff check, direct SDK backpressure probes, and multiple clean-ending rounds of correctness, compatibility, and resource-bound review.
Risk & Scope
qwen servedaemon-owned ACP channels opt in. They now retire the exact workspace generation for malformed envelopes, unexpected clean EOF, unmatched response IDs, known-method schema violations, or handler/outbound/prepared-response saturation. Public and standalone ACP stream and bridge defaults remain unchanged.Linked Issues
Follow-up to #8911. Part of #8091. Related to #8051.
中文说明
本 PR 做了什么
本 PR 是 #8911 的补强，关闭 daemon ACP bounded transport 周边的资源所有权缺口。daemon-owned channel 现在会在 ACP SDK 分发前校验有界 JSON-RPC envelope；对 active handler、prepared response、进入 SDK 前的 outbound operation 和 outstanding request ID 同时施加数量与保守字节计费；当协议、序列化、EOF 或 admission 失败使 transport 不可用时，回收准确的 workspace channel generation。
bridge 会在 child 完成终止宽限期之前将失败 channel 标记为不可用，阻止 initialize、create、restore、attach、prompt 和 status 工作继续复用该 generation，并在投递失败时回滚 provisional session ownership。fatal frame 之前已接纳的完整 frame 仍按顺序投递。bounded message 和 client error 使用仅元数据的 inspect 结果，避免 ACP SDK 独立记录 child-controlled payload。
公共和 standalone ACP stream 仍为显式 opt-in：不提供 limits 或 transport guard 的调用方保留原有 queue、EOF、parse log 和 error wire 行为。
为什么需要
#8911 约束了原始 NDJSON assembly 和 decoded
ReadableStream，但 ACP SDK 会立即 drain readable 而不等待 request handler，并维护私有 write queue 和 pending-response queue。因此 child 仍可通过慢 handler 绕过 stream watermark，在持续发送 request 的同时停止读取 stdin，用小 request 生成大 response，或永不响应 daemon 的重复 request。transport failure 也可能在 child 退出之前先关闭 SDK connection，而 bridge 仍把该 generation 视为可复用。这些路径会在原始 queue 计费之外保留 payload、Promise、session 和 child process。将所有权延长到 handler 完成、response 投递、outbound request 结算以及准确 generation teardown，可防止单个异常 workspace 无限增长 daemon 资源，也避免其他请求继续卡在已失效 connection 上。
Reviewer 测试计划
如何验证
使用真实 ACP SDK connection 和阻塞的 writable，发送超过 256 个 child-to-daemon request，并让 handler 返回 response。确认前 256 个保持有界，下一个 admission 只回收对应 channel。随后在 child 不读取 stdin 时重复发送 daemon-to-child request 与 notification；确认 admission 在进入 SDK 私有 write queue 前发生，并且 notification 仅在本地投递后释放、request 仅在响应或确定失败后释放。
让仍保持进程存活的 child 发送 malformed known-method params、超大协议 scalar、未知 response ID、未完成 EOF 和干净 stdout EOF。确认每个 daemon-owned transport 只报告一次 typed、metadata-only failure，立即将准确 channel generation 标记为不可用，终止其 tracked child，并保持另一个 workspace 可响应。确认合法匹配 response 可以先于父进程 write callback 到达而不会被拒绝。
在 transport failure 与操作并发时覆盖 initialize、fresh session creation、existing attach、load/resume、branch cleanup、cwd change、shell-history injection 和 extension refresh。确认 teardown 后不残留 provisional attach/session ownership、admission lease、SDK queue entry 或长寿命 refresh reaction。
最后，在不提供 limits 或 transport guard 的情况下构造公共 stream 和 bridge，确认 legacy EOF、parse logging 与 error response shape 保持不变。
证据（Before & After）
N/A — 非 UI 的 daemon transport 与生命周期加固。
测试平台
环境（可选）
macOS arm64、Node.js 24.12.0、npm 10.9.8。已验证 727 个 ACP bridge 聚焦测试、243 个 daemon runtime 测试、完整 build、完整 typecheck、完整 lint、Prettier 检查、diff 检查、直接 SDK backpressure 探针，以及多轮最终 clean 的 correctness、compatibility 和 resource-bound review。
风险与范围
qwen servedaemon-owned ACP channel 会 opt in。malformed envelope、意外 clean EOF、不匹配的 response ID、known-method schema violation 或 handler/outbound/prepared-response 饱和现在会回收准确 workspace generation。公共和 standalone ACP stream 与 bridge 的默认行为保持不变。关联 Issue
#8911 的 follow-up，属于 #8091 的一部分，与 #8051 相关。