fix(daemon): Cancel timed-out session initialization - #10268
Conversation
E2E test reportVerified on rebased commit
The complete built-daemon scenario could not run because |
github-actions
Bot
added
the
review/self-reported
|
Gate re-pass on head Template: complete ✓ Problem: unchanged and real — an observed bug, not hardening. The public Direction: aligned. This closes a genuine daemon correctness hole (a slow Size: core + cross-package change ( Approach: scope is justified rather than bloated. Child-side cancellation alone cannot cover older children that ignore the deadline, so the bridge's late-settlement containment (exact-ID close, fences, bounded quarantine) is part of the stated goal, not scope creep. The design doc and protocol-doc updates match the code. Nothing I would cut. Risk: Stage 1e matches 中文说明在 head 模板： 完整 ✓ 问题： 不变且真实 —— 已观测到的 bug，不是理论加固。公开的 方向： 对齐。修补真实的 daemon 正确性漏洞（缓慢的 规模： 核心 + 跨包改动。731 行生产逻辑（bridge.ts 517、acpAgent.ts 75、client.ts 40、config.ts 25、dispatch.ts 16、error-response.ts 13、其余小文件 45），测试 1006 行，文档 97 行。类型为 方案： 范围合理而非膨胀。仅靠子进程侧取消无法覆盖忽略 deadline 的旧子进程，因此 Bridge 的迟到 settle 约束（精确 ID 关闭、fence、有界隔离）是目标的一部分，不是夹带。设计文档与协议文档更新与代码一致。没有可砍的部分。 风险： Stage 1e 命中 — Qwen Code · qwen3.8-max Reviewed at |
Code review — round 3 (head
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Real daemon E2E / Java 11 | success |
| Serve A/B (ubuntu-latest, Node 22.x) | success |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| ubuntu-latest / Java 11, 17, 21 · macos-latest / Java 21 · windows-latest / Java 21 (SDK Java) | success |
| Dependency CVE audit · Secret scan (TruffleHog) | success |
| Integration Tests (CLI, No Sandbox) | skipped (fork gating) |
| Test (macos-latest, Node 22.x) · Test (windows-latest, Node 22.x) | skipped (fork gating) |
The central claim is behavioural and the green unit suite alone does not pin it (a suite can pass with the containment removed). Sandboxed verification would settle the rest: @qwen-code /verify — that a timed-out init really reaps the hook process tree, that an older child settling late is closed by exact ID without touching a healthy sibling, and that cleanup-uncertainty fails closed only for fresh admission. The author lacks write access, so this is a sponsored run (a maintainer's @qwen-code /verify approves the head it was written against; the run carries a pre-execution risk screen and a full workspace wipe — read its report with the same skepticism as the fork's own CI logs). A verification run is already in flight on this head (workflow run); its report lands in this thread. Prior /verify rounds on earlier heads passed (4746 then 4824 scripted assertions, 0 failures).
中文说明
代码审查 —— 第 3 轮（head 23a2637d）
第 2 轮的 head 之后又有三次推送：R4-1 公开超时映射、一批 Critical 修复（按 token 的 overdue 跟踪、精确 ID 所有权预留、协议表重试提示）、以及 rebase 到 main。我在该 head 上重新通读了全部生产代码 diff，并对照代码（而非作者的表格）验证了此前的 Critical。
本轮核心修复引入的顺序风险处理正确。 SessionRestoreTimeoutError extends BridgeTimeoutError，粗心的匹配会把 restore 超时静默重映射为 init_timeout。两个传输层都先匹配子类：sendBridgeError 先判 SessionRestoreTimeoutError（error-response.ts:200 对 :217），toRpcError 的 switch 先命中 'SessionRestoreTimeoutError'（dispatch.ts:824 对 :841），且子类设置了独立 name。该映射也与现有错误分类一致 —— mapDomainErrorToErrorKind 本就把任何 BridgeTimeoutError 映射为 init_timeout。
此前的 Critical 已验证在该 head 修复： drain 路径判定回归（R1-11 类）全部改走 channelIsCondemned()；channel 级 overdue 布尔量已被按 token 的 overdueAbandonedNewSessions 集合取代（我独立 grep 了 head 文件：旧标志 0 次出现）；迟到清理的 ID 冲突会先等待在途 restore/spawn 的 settlement，且跳过已被活跃 session 拥有的 id，不会自锁；awaiting_abandoned_cleanup 的重试提示在 409 与 503 上都跟随超时操作的预算。
可信路径门控正确。 deadline 仅在受管父进程能力握手后读取；受信任但格式非法的值在创建任何状态前即以 invalid_session_initialization_deadline 失败；不受信调用方的 deadline 元数据被忽略（均有新测试覆盖）。信号是请求级的、定时器全部 unref、finally 中清理、不保存到已发布的 Session。
本轮无新 Critical。 其余均为已披露的非阻塞项：R4-2/R4-3 由作者按评审变更规则延期；第 5 轮 /review 的 R5-1（504 契约的 route 级钉桩）为建议级；beyond the timer range 时钟抖动 flake 此前已被接受为非阻塞。
时序图见英文部分（调用方 → Bridge → ACP 子进程的超时后迟到结算与有界清理路径）。
测试证据 —— 该 commit 上 PR 自身的 CI
无人值守运行：未构建或执行任何 PR 代码，以下证据来自 API 拉取的 23a2637d 的 PR 自身 CI，且全部出结果（无 pending）。Integration Tests (CLI, No Sandbox) 与 macOS/Windows 单元测试矩阵为 skipped（fork PR 门禁，非失败）；作者的本地 macOS 数字是其自述，未在此独立复跑。CI 表格见英文部分（机器可读区域内）。
核心主张是行为性的，仅凭绿色单测无法钉桩。沙箱验证可补齐：@qwen-code /verify —— 验证超时初始化确实回收 hook 进程树、旧子进程迟到 settle 被按精确 ID 关闭且不伤及健康兄弟、清理不确定时仅对新准入失败关闭。作者无写权限，因此为 sponsored run（由维护者的 @qwen-code /verify 批准，运行前有安全筛查与完整工作区清理 —— 请像对待 fork CI 日志一样审慎阅读其报告）。当前已有一个针对该 head 的验证运行在进行中，报告将发布在本线程；此前两轮 /verify（在更早 head 上）分别以 4746、4824 条断言全部通过。
— Qwen Code · qwen3.8-max
Reviewed at 23a2637d261981b9365f24af18313f07d81f0807 · re-run with @qwen-code /triage
|
Confidence: 3/5 — clean review at this head; the 3 is pure policy, not doubt. The Stage 0 core-size escalation (731 production lines across Honest read after three rounds: this PR has earned its approvals. The problem is observed, not hypothesized — a timed-out What my own pass added this round: a full re-read of the production diff at Why I'm still not approving: policy, stated plainly. A fork PR carrying 500+ production lines into core paths is one the bot does not auto-approve regardless of how clean every stage looks — that is the escalation working as designed, and nothing about this round changes it. CI is fully settled and green at the reviewed commit, so there is no pending-work reason to wait either; the only remaining step is human. ⏸️ Deferring to @yiliang114 — the cap is policy, the code is ready by my read, and your approval already stands on this exact commit. What's left is a human call: 中文说明置信度：3/5 —— 该 head 上审查干净；3 分纯属策略封顶，而非存疑。Stage 0 核心规模升级（731 行生产代码横跨 三轮下来的诚实判断：这个 PR 配得上它的批准。问题是观测到的而非假设 —— 超时的 本轮我自己补充的：在 为何仍不批准：策略，直说。携带 500+ 生产行核心路径改动的 fork PR，无论各阶段多干净，机器人都不自动批准 —— 这正是升级机制的设计意图，本轮没有任何因素改变它。CI 在该 commit 上已全部出结果且全绿，也不存在等待未决检查的理由；唯一剩下的步骤是人工。 ⏸️ 转交 @yiliang114 —— 封顶是策略，代码以我的判断已经就绪，你的批准已经落在该 commit 上。剩下的是人工决定： — Qwen Code · qwen3.8-max Reviewed at |
🩺 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 |
tmux E2E test report (head
|
|
@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: 4746 passed · 0 failed · 4746 total Flakiness gate: ✅ 5 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4746 通过 · 0 失败 · 4746 总计 抖动门：✅ 5 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10268 Deep Verification ReportVerdict: 中文摘要
Central claim and A/B proofCentral claim: when a managed ACP Harness:
Head run: 26/26 scripted assertions; base run: 25/25 (base-side cells assert the bug). Empty-channel branch ( Corrections
FindingsNone blocking.
Mutation matrix (vacuity) — zero survivorsAll mutants applied by exact string replacement in scratch trees, run through the same vitest
Hook process-tree reclamation (real processes)
A/A by construction: Targeted gates
Gate liveness: each package's gate was proven live by a red mutant run in the same file Not covered
MethodologyEnvironment: Flakiness gate logEvidence 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.
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.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：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)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the current review round in e279783.
Verification:
The repository-wide build and CLI typecheck still encounter the known pre-existing Ink selection API/type errors in unchanged UI files; no evidence connects those baseline failures to this branch. Replied to and resolved 16 review threads. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Deep-tier review of the session initialization deadline and late-cleanup compatibility layer. No blocking findings.
CI bot R1-11 (Critical) — REFUTED: The CI bot claimed channelIsCondemned "still recognizes only isQuarantined/restoreSettlementOverdue". Looking at the PR diff, channelIsCondemned is a new function introduced by this PR at bridge.ts:3482–3489, and it correctly includes all four states: isQuarantined, restoreSettlementOverdue, newSessionCleanupFailed, and newSessionSettlementOverdue. The old two-condition inline check at the former lines 757–758 was replaced by the new channelIsCondemned call. Both call sites that the CI bot flagged (closeIfChildUnheld agentCloseTimeoutMs and entryIsAutoCloseCandidate capability carve-out) already use channelIsCondemned in the submitted code. The "probe" findings the CI bot reported appear to reflect a pre-PR baseline, not the current diff.
Core mechanism — verified correct:
freshSessionBlocker()(lines 2646–2662) checks all four condemned states in order, returning the rightBridgeChannelUnavailableReason.newSessionSettlementOverdueis cleared at line 4745 whenci.unsettledAbandonedNewSessions.size === 0, i.e., after the last abandoned session settles. Correct.newSessionCleanupFailedis set and never cleared by design — the channel is condemned until drained and reaped. Correct.channelIsCondemnedconsistently used at all three drain-path call sites (lines 3043, 3151, 3202).
Suggestions (deferred, not blocking):
- R1-9: Load/restore-path admission counters for
abandonedNewSessionSettlements.sizelack test coverage. - R1-14: Child-side-deadline timeouts don't emit the
session.new.public_resulttelemetry event or thedecision=stderr line that the bridge-timer path emits; diagnostic gap. - R1-16: Caller-supplied ID fence during abandoned newSession gives a bare
RestoreInProgressErrorwithout theawaiting_abandoned_cleanupreason or budget-derived retry hint. - R1-1/R1-3/R1-12/R1-13: Documentation overclaims quarantine as "temporary" in contexts where
newSessionCleanupFailedis permanent until channel death.
CI disclosure: build-cli is SKIPPED on this head — no unit test run evidence for any of the concurrency or cancellation changes. web-shell E2E Smoke passes; Desktop Shell, Test, and daemon E2E checks did not run.
Reviewed with AI assistance.
|
@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: 4824 passed · 0 failed · 4824 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4824 通过 · 0 失败 · 4824 总计 抖动门： Verification reportPR 10268 Deep Verification Report (follow-up round)Verdict: This is a follow-up round: the previous round verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定）本轮为复验轮：上一轮在 head
Previous-finding status (follow-up round)
Central claim and A/B proofCentral claim: when a managed ACP Harness:
Head: 38/38 scripted assertions ×2 runs. Base: 24/24 ×2 runs. Discarded base-arm predictions (reported, not buried) — my first base run had four
Corrections
FindingsNone blocking.
Mutation matrix (vacuity) — 13/13 killed, zero survivorsAll mutants applied by exact string replacement at the new head, run through the package's
Adjudicated probe: the first M-F2 variant targeted the branch-admission cap site Hook process-tree reclamation (real processes, A/A)
A/A by construction: Targeted gates
Gate liveness: proven in the same round before any green result was cited. The mutation Not covered
MethodologyEnvironment: 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. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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 — reached the 5-round cap still reporting (rounds 3–5 each found new gaps); terminated by cap, not by convergence.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.test.ts:12801 — [probe] absolute-deadline contract asserted only as expect.any(Number)packages/acp-bridge/src/bridge.ts:2668 — [probe] per-reason retry-hint ternary has no observing testpackages/acp-bridge/src/bridge.ts:13569 — [review] shutdown await of abandoned new-session settlements is untestedpackages/cli/src/acp-integration/acpAgent.test.ts:2318 — [review] createAndStoreSession's geminiClient.initialize signal is untestedpackages/acp-bridge/src/bridge.ts:13569 — [probe] shutdown settlement snapshot misses in-flight spawns abandoned laterpackages/core/src/core/client.test.ts:1106 — [probe] post-success throwIfAborted() in fireSessionStartHook is untestedpackages/core/src/core/client.test.ts:1121 — [probe] GeminiClient startChat signal seams have no driving testpackages/acp-bridge/src/bridge.test.ts:21406 — [probe] acceptance side of the timer-range boundary is unpinnedpackages/acp-bridge/src/bridge.ts:8972 — [probe] transient capacity double-count between abandonment and inFlightSpawns.delete
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — reached the 5-round cap still reporting (rounds 3–5 each found new gaps); terminated by cap, not by convergence。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 9 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the current Critical review finding in e8c9417.
To honor the review's stated convergence posture and the repository's review-churn rule, this push intentionally contains no Suggestion-only changes. R1-1, R1-6, R2-2 through R2-7, and the review's explicitly disclosed deferred audit gaps remain recorded for follow-up rather than widening this PR further. |
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.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- deadline meta contract asserted only as expect.any(Number) (bridge.test.ts:12801) — already reported (round-2 deferred list, review 5043485951)
- per-reason retry-hint ternary has no observing test on the 503 path (bridge.ts:2668) — already reported (round-2 deferred list, review 5043485951)
- createAndStoreSession post-config signal checkpoints untested (acpAgent.test.ts:2318) — already reported (round-2 deferred list, review 5043485951)
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 3, not a blocker) — recorded, not requested in this round:
docs/developers/qwen-serve-protocol.md:2133 — [review] R1-1 settlement-overdue paragraph still says "settles cleanly first" while the shared finally clears on either outcome — still stands, deferred (convergence posture)packages/core/src/config/config.test.ts:4251 — [review] R1-6 post-Gemini signal cancellation checkpoints still lack isolated coverage — still stands, deferred (convergence posture)packages/cli/src/acp-integration/acpAgent.test.ts:2363 — [review] R2-2 timer-range invalid-deadline case still sits exactly 1 ms past the boundary (wall-clock race) — still stands, deferred (convergence posture)packages/acp-bridge/src/bridge.test.ts:13048 — [review] R2-3 new-session settlement telemetry variants still unpinned (late_result polarity, cleanup_result closed/quarantined, settlement_overdue) — still stands, deferred (convergence postur…docs/developers/qwen-serve-protocol.md:2142 — [review] R2-4 settlement-overdue clearing on late FAILURE settlement still has no test — still stands, deferred (convergence posture)packages/acp-bridge/src/bridge.test.ts:13162 — [review] R2-5 capacity guards' abandoned-settlement term still has no branchSession witness — still stands, deferred (convergence posture)packages/acp-bridge/src/bridge.test.ts:13152 — [review] R2-6 still no test abandons two overlapping newSessions on one channel (multi-token overdue guard) — still stands, deferred (convergence posture)packages/acp-bridge/src/bridge.test.ts:12944 — [review] R2-7 requested-id spawn fence still only driven via loadSession, never resumeSession — still stands, deferred (convergence posture)
Convergence: round 3 posted 13 inline comment(s), 13 of them reported for the first time; the previous round posted 9 (8 new). Findings keep coming back to the same files: packages/acp-bridge/src/bridge.test.ts (findings in round 2; 6 more now); packages/acp-bridge/src/bridge.ts (findings in round 2; 3 more now); packages/cli/src/acp-integration/acpAgent.test.ts (findings in round 2; 2 more now), and 1 more file(s). The rate of new findings is not falling. 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. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 13 条行内评论，其中 13 条是首次提出；上一轮发布了 9 条（其中 8 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/bridge.test.ts（第 2 轮已出过发现，本轮又有 6 条）；packages/acp-bridge/src/bridge.ts（第 2 轮已出过发现，本轮又有 3 条）；packages/cli/src/acp-integration/acpAgent.test.ts（第 2 轮已出过发现，本轮又有 2 条），另有 1 个文件。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-8 registered-ID reclamation coverage — already reported at packages/acp-bridge/src/bridge.ts:4657 (comment 3875448773)
- R3-7 late newSession rejection cleanup coverage — already reported at packages/acp-bridge/src/bridge.ts:4932 (comment 3875448781)
- R3-9 resolved SessionStart hook cancellation coverage — already reported at packages/core/src/core/client.test.ts:1126 (comment 3875448857)
Convergence: round 3 posted 9 inline comment(s), 9 of them reported for the first time. Findings keep coming back to the same files: packages/acp-bridge/src/bridge.ts (findings in round 2; 3 more now); packages/cli/src/acp-integration/acpAgent.test.ts (findings in round 2; 2 more now); docs/developers/qwen-serve-protocol.md (findings in rounds 1, 2; 1 more now), and 1 more file(s). (Evidence: the previous round was recovered from a marker this account did not post, so those rounds may not be this account's own.) 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.)
[Critical] [probe] R3-1: Late abandoned-session cleanup can close a concurrently restored session before the bridge registers it. An anonymous newSession times out, then an in-flight loadSession creates ID X in the child but has not returned to bridge registration. If the abandoned newSession returns X first, byId does not yet contain X, so cleanup sends sessionClose(X); the restore then registers an already-closed session. PR probe: closeCalls = ["collision-id"]; the restore subsequently resolved and bridge.sessionCount became 2. Adding an inFlightRestores.has(lateSessionId) discriminator prevented the close and flipped the probe. Coordinate abandoned cleanup with the same per-ID ownership used by in-flight restore/spawn registration; do not close an ID whose ownership is live or transferring. Add a deferred restore/newSession collision test that resolves the abandoned call with the restore ID before registration and proves no close is sent and the restored session remains usable.
[Critical] [probe] R3-3: The empty-channel timeout path waits for channel termination before returning the public initialization timeout. When the first newSession exceeds initializeTimeoutMs and channel.kill() never settles, the timeout catch awaits kill and its finally path also awaits teardown through reaping, so spawnOrAttach remains pending instead of promptly returning BridgeTimeoutError. With kill returning a never-settling promise, the unmodified PR remained pending; making both timeout teardown calls fire-and-forget made the focused test pass (1 passed, 816 skipped). Mark the channel dying synchronously, retain background reap bookkeeping, and return the timeout without awaiting teardown. Add an empty-channel test with a non-settling kill promise that proves spawnOrAttach rejects after the timer while teardown remains pending.
[Critical] [probe] R3-4: Writer cleanup failure replaces the initialization deadline error with session_writer_unavailable. If the deadline aborts after writer activation and closeSessionWriter rejects during failed-initialization cleanup, initializeOnce wraps both errors in SessionWriterUnavailableError, so the Agent and Bridge no longer expose the session_initialization_timeout/init_timeout contract. The unmodified PR returned SessionWriterUnavailableError instead of the initialization deadline error; preserving options.signal.reason made the focused test pass (1 passed, 582 skipped). Preserve the signal reason as the request result and retain or log cleanup failure separately. packages/acp-bridge/src/bridge.ts maps only SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND to BridgeTimeoutError, so cleanup must not replace that error kind. Add a Config test that aborts after writer activation while writer close rejects and proves the exact abort reason survives ACP error mapping.
中文说明
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛情况：第 3 轮发布了 9 条行内评论，其中 9 条是首次提出。发现反复回到同一批文件：packages/acp-bridge/src/bridge.ts（第 2 轮已出过发现，本轮又有 3 条）；packages/cli/src/acp-integration/acpAgent.test.ts（第 2 轮已出过发现，本轮又有 2 条）；docs/developers/qwen-serve-protocol.md（第 1、2 轮已出过发现，本轮又有 1 条），另有 1 个文件。（证据说明：上一轮的数据来自并非本账号发布的标记，上述轮次可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
[Critical] [probe] R3-1: Late abandoned-session cleanup can close a concurrently restored session before the bridge registers it. An anonymous newSession times out, then an in-flight loadSession creates ID X in the child but has not returned to bridge registration. If the abandoned newSession returns X first, byId does not yet contain X, so cleanup sends sessionClose(X); the restore then registers an already-closed session. PR probe: closeCalls = ["collision-id"]; the restore subsequently resolved and bridge.sessionCount became 2. Adding an inFlightRestores.has(lateSessionId) discriminator prevented the close and flipped the probe. Coordinate abandoned cleanup with the same per-ID ownership used by in-flight restore/spawn registration; do not close an ID whose ownership is live or transferring. Add a deferred restore/newSession collision test that resolves the abandoned call with the restore ID before registration and proves no close is sent and the restored session remains usable.
[Critical] [probe] R3-3: The empty-channel timeout path waits for channel termination before returning the public initialization timeout. When the first newSession exceeds initializeTimeoutMs and channel.kill() never settles, the timeout catch awaits kill and its finally path also awaits teardown through reaping, so spawnOrAttach remains pending instead of promptly returning BridgeTimeoutError. With kill returning a never-settling promise, the unmodified PR remained pending; making both timeout teardown calls fire-and-forget made the focused test pass (1 passed, 816 skipped). Mark the channel dying synchronously, retain background reap bookkeeping, and return the timeout without awaiting teardown. Add an empty-channel test with a non-settling kill promise that proves spawnOrAttach rejects after the timer while teardown remains pending.
[Critical] [probe] R3-4: Writer cleanup failure replaces the initialization deadline error with session_writer_unavailable. If the deadline aborts after writer activation and closeSessionWriter rejects during failed-initialization cleanup, initializeOnce wraps both errors in SessionWriterUnavailableError, so the Agent and Bridge no longer expose the session_initialization_timeout/init_timeout contract. The unmodified PR returned SessionWriterUnavailableError instead of the initialization deadline error; preserving options.signal.reason made the focused test pass (1 passed, 582 skipped). Preserve the signal reason as the request result and retain or log cleanup failure separately. packages/acp-bridge/src/bridge.ts maps only SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND to BridgeTimeoutError, so cleanup must not replace that error kind. Add a Config test that aborts after writer activation while writer close rejects and proves the exact abort reason survives ACP error mapping.
— gpt-5.6-sol@954e5164 via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Critical review fixes pushed in
Verification: ACP bridge full file 820/820; Config full file 583/583; focused exact-head regressions 8/8 and 2/2; both affected package builds and typechecks passed; targeted ESLint and |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Deep（并发 session 初始化 + overdue 状态机）— 增量 e2797834 → 7a8b1fe0
已核验内容
R3-3（channel-wide overdue flag 在唯一超时请求 settle 后不清除）
核心修复：将 newSessionSettlementOverdue: boolean 改为 overdueAbandonedNewSessions: Set<symbol>，按 token 粒度跟踪：
- 超时时：
ci.overdueAbandonedNewSessions.add(token)替代设置全局 boolean - settle 时：
ci.overdueAbandonedNewSessions.delete(token)逐个清除 - 判断时：
ci.overdueAbandonedNewSessions.size > 0替代ci.newSessionSettlementOverdue
旧逻辑：flag 只在 unsettledAbandonedNewSessions.size === 0 时才清除（需要全部 settle），导致 A/B 两个超时请求中 A 先 settle 后 flag 仍保持。新逻辑：A settle → A 的 token 被 delete，flag 随即变 false（若 B 未 settle 则 B 的 token 仍在 set 中）。
测试 tracks settlement overdue state per abandoned newSession 验证：两个超时请求中，lateA.reject 后 channel 立即恢复可用（lateB 仍 pending 时通道仍 condemned，lateB.reject 后恢复）。✓
R3-10/R3-11 测试新增
returns an empty-channel timeout without waiting for channel kill：验证kill()永远 pending 时 timeout 仍正常返回BridgeTimeoutError而不阻塞 ✓does not close a late newSession id owned by an in-flight restore/does not close a late newSession id owned by an in-flight spawn：验证 session id 归属判断正确 ✓
R3-12（config.ts：writer cleanup 失败覆盖了 init deadline error）
在 closeSessionWriter catch 中增加前置检查：若 signal.aborted 且 abort reason 包含 original error，则 log close 失败后 throwIfAborted()——确保初始化超时错误正确传播，不被 close error 覆盖。✓
CI
1 success，1 in_progress（review-pr），28 skipped。核心测试无失败。
无阻塞项。
Propagate the daemon initialization deadline through ACP session startup and SessionStart hooks, while containing late results from older agents without disrupting shared-channel siblings. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
7a8b1fe to
9bfba01
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）为单个提交。 |
|
Rebased onto current The three content conflicts were mechanical adaptations to the merged Gemini→Llm rename: Verification on the rebased head: focused Core 8/8 and CLI ACP 9/9 passed; the earlier full exact-content runs passed Core 957/957; Core and ACP Bridge typechecks passed; relevant ESLint and |
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:
- session.new.late_result / session.new.settlement_overdue telemetry unpinned — already reported (round-2 R2-3, comment 3873973810)
- deadline meta contract asserted only as expect.any(Number) — already reported (round-2 deferred list, review 5043485951)
- per-reason retry-hint ternary has no observing test — already reported (round-2 deferred list, review 5043485951)
- createAndStoreSession pre-publication signal checkpoints untested — already reported (round-2 deferred list, review 5043485951; round-3 comment 3875658117)
- id-fence tests negative-only (id_reclaimed settlement unpinned) — already reported (round-3 R3-1, comment 3875448773)
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.
Convergence: round 4 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 13 (13 new). Findings keep coming back to the same files: packages/acp-bridge/src/bridge.test.ts (findings in round 3; 2 more now); docs/developers/qwen-serve-protocol.md (findings in round 2; 1 more now). (Evidence: the previous round was recovered from a marker this account did not post and merged over this account's own entries, so some of those rounds and its counts may not be this account's own.) 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.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 5 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛情况：第 4 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 13 条（其中 13 条首次提出）。发现反复回到同一批文件：packages/acp-bridge/src/bridge.test.ts（第 3 轮已出过发现，本轮又有 2 条）；docs/developers/qwen-serve-protocol.md（第 2 轮已出过发现，本轮又有 1 条）。（证据说明：上一轮的数据来自并非本账号发布的标记，并与本账号自己的条目合并，上述轮次与其计数中的部分可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review follow-up for 23a2637
Verification: 44 focused mapper tests passed; the existing REST restore-timeout route test passed and preserved restore_timeout; focused Prettier, ESLint, and git diff checks passed. CLI typecheck remains blocked only by the pre-existing unchanged Ink selection API/type failures. |
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.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-2 staggered overdue test cannot kill the clear-all mutant (bridge.test.ts:13336) — still stands; already reported (round-4 comment 3878640381), author deferred
- R4-3 cleanup-failed recovery lacks channel-recycle mutation coverage (bridge.test.ts:13184) — still stands; already reported (round-4 comment 3878640386), author deferred
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 5": traced neither the session-creation route's catch block (which responder it forwards bridge errors to) nor the RPC newSession dispatch path — this is why Find…; "agent 1a": did not execute the two new test files — the worktree has no built package dist/ outputs (vitest's globalSetup guard would halt the run) and building in the s….
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 5"：traced neither the session-creation route's catch block (which responder it forwards bridge errors to) nor the RPC newSession dispatch path — this is why Find…；"agent 1a"：did not execute the two new test files — the worktree has no built package dist/ outputs (vitest's globalSetup guard would halt the run) and building in the s…。
— qwen3.8-max via Qwen Code /review (v0.22.2)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent pass at 23a2637, focused on the cancel-vs-complete race (prior 51 threads all resolved; bot CHANGES_REQUESTED treated as stale). No substantiated findings. The design avoids the naive cancel race properly: a timed-out newSession is not force-cancelled mid-flight; it is recorded (recordNewSessionPublicTimeout), the channel decision is kill_empty when there is no other work or fence_shared otherwise, and the abandoned spawn is tracked via inFlightSessionIdReservations settlement promises with a settlement grace timer (newSessionSettlementGraceMs = initTimeoutMs) that marks the channel overdue and refuses fresh sessions until the abandoned init settles or drains — mirroring the existing restore-settlement machinery, so a late-completing init cannot orphan a session id or collide with a retry. Timers are unref-ed and cleared on channel death; initializeTimeoutMs is validated as a positive integer within the setTimeout range. Residual (not a finding): I did not trace reapPendingEmptyChannel end-to-end against concurrent spawn admission under fence release; the added bridge tests (672 lines) target that area.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approving. Re-verified at head 23a2637: the channel-wide overdue boolean is gone (0 occurrences of newSessionSettlementOverdue) and replaced by the per-token overdueAbandonedNewSessions Set (bridge.ts:947, add at :3541, per-token delete in the settle finally at :4779, admission reads size>0 at :2657/:3487) — exactly the staggered-two-token defect class reported. Both doc findings are fixed too (recovery-point semantics now distinguish late-failure vs late-success cleanup at protocol.md:2175/2184 and error-taxonomy.md:62; the fence-table row no longer claims a blanket 504). All 51 threads resolved; the test-pin asks from the last round are in (id_reclaimed, abandoned-phase reject, drainTimeoutMs spot-checked). Known leftover: the 'beyond the timer range' real-clock flake (acpAgent.test.ts:2362) remains, previously accepted as non-blocking. CI fully green.
|
@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: 4831 passed · 0 failed · 4831 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4831 通过 · 0 失败 · 4831 总计 抖动门： Verification reportPR 10268 Deep Verification Report (follow-up round 2)Verdict: This is a follow-up round: the previous round verified head 中文 — 判定：✅ 通过 · 可合入（agent 判定）本轮为复验轮（第二次）：上一轮在 head
Previous-finding status (follow-up round)
Central claim and A/B proofCentral claim: when a managed ACP Harness:
Head: 59/59 scripted assertions. Base: 37/37. Discarded base-arm predictions (reported, not buried) — two harness-prediction bugs of my Delta focus: the four tests added since the previous roundbridge.test.ts grew 816 → 820 (+4). The tests covering the settlement-ownership and
M-H2 adjudication (harness error found and corrected): the first-draft M-H2 deleted the Mutation matrix (vacuity) — 13/13 killed, one adjudicated survivor, 3/3 delta killedAll mutants applied by exact string replacement at the new head, run through the package's
Positive control for the survivor: the same vitest invocation shape killed 15 other mutants in Hook process-tree reclamation (real processes, A/A)
A/A by construction: Targeted gates
Gate liveness: the mutation matrix turned suites red in the identical files as the mutants Corrections
FindingsNone blocking.
Flakiness gate5 rounds × 6 changed test files, one file per vitest invocation: 30/30 runs green, Unlike the previous round (4 of 5 rounds fit its 15-minute budget), this round completed all Not covered
MethodologyEnvironment: Flakiness gate logEvidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Round 3 — new head 23a2637d, independent re-review.
Prior findings: R1 and R2 both dismissed after author updated. This review is fresh against current head.
Scope
Tier: Deep. Changes touch ACP session lifecycle protocol, timer ordering, late-settlement quarantine state machine, and AbortSignal cancellation chain. All 20 changed files were read; cross-file context was fetched for the key contracts.
CI:
| Job | Result |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ pass (34m26s) |
| Test (macos-latest, Node 22.x) | |
| Test (windows-latest, Node 22.x) | |
| Integration Tests (CLI, No Sandbox) | |
| Real daemon E2E / Java 11 | ✅ pass |
Platform-specific timer resolution and path handling are unreviewed dimensions. Author confirms macOS-only manual testing.
What I verified
Contract chain: Bridge sends SESSION_INITIALIZATION_DEADLINE_META_KEY = Date.now() + initTimeoutMs; agent reads it only when isTrustedManagedParent(), validates positive safe integer ≤ 2^31-1, converts to remainingMs = raw - Date.now(), arms AbortController. Forged deadlines from untrusted callers are silently ignored — confirmed by the untrusted-parent test.
Signal propagation: agent.newSession → config.initialize → llmClient.initialize → startChat → fireSessionStartHook → hookSystem.fireSessionStartEvent. Signal passed at each hop. fireSessionStartHook catch block calls signal?.throwIfAborted() before warn+swallow, which surfaces cancellation even when the hook throws independently (tested at client.test.ts:1108).
Session publication order: Abort checkpoint after registerCreateSubSessionTool in createAndStoreSession fires before this.sessions.set, consistent with the comment "BEFORE the session is published." Abort here leaves nothing in the session map; settleAbandonedNewSession(ci, token, undefined, ...) exits early in the finally block cleanly.
Quarantine state machine: channelIsCondemned covers all four states. Used in confirmChildUnheld, closeSessions, and maybeCloseIdleSession — verified by grep.
Session cap accounting: abandonedNewSessionSettlements.size at three cap-check sites. Test at bridge.test.ts:12761 confirms a timed-out spawn counts against maxSessions until abandoned settlement resolves.
initTimeoutMs validation: Tightened to positive integer ≤ 2^31-1. Required for Date.now() + initTimeoutMs arithmetic.
settleAbandonedNewSession loop: spawnOwner && lateSessionId !== requestedSessionId guard correctly waits for an in-flight spawn owning the id, passes through when the late id IS the requested id. Tested by "does not close a late newSession id owned by an in-flight spawn."
_meta now unconditional: Backward-compatible — legacy agents ignore unknown fields.
Cross-check against existing reviews
Confirmed:
bridgeErrors.ts JSDoc ("settlement-overdue states may clear when the abandoned request settles") — For late successes, the state actually clears in settleAbandonedNewSession's finally block only after the sessionClose call completes, not at raw request settlement. "When the abandoned request settles" understates this for the success path. The protocol doc 503-entry is precise ("after a late success completes its exact-ID cleanup"); this JSDoc should match.
docs/developers/qwen-serve-protocol.md POST /session section — mentions "init timeout" in prose at the coalescing note, but has no formal 504 init_timeout error entry, even though error-response.ts now returns 504 / init_timeout / retryable:true / Retry-After for BridgeTimeoutError. The error contract and Retry-After semantics for timed-out session creation are not discoverable from the spec.
Cannot rule:
R2-1 round-3 fix-induced finding — I traced the Retry-After plumbing through error-response.ts and dispatch.ts and do not see an obvious contradiction at head, but the original R2-1 inline comment is not visible to me. Recording as a miss I cannot rule on.
My own findings
None beyond the two confirmed above (documentation precision).
Reviewed with AI assistance.
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Round 3 documentation follow-up for 23a2637
No correctness, security, data-loss, or regression defect was identified, so this review round does not change the branch. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse-audit chunk 5 round 4 — auditor failed twice; round 5 returned a substantive dry receipt.
Not reviewed: reverse-audit chunk 7 round 3 — receipt wording did not satisfy retirement certification; rounds 4 and 5 returned substantive dry receipts.
Not reviewed: "agent verify (round 2)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
docs/developers/qwen-serve-protocol.md:2175 — [review] The recovery contract omits the id_reclaimed path that skips exact-ID cleanupintegration-tests:1 — [review] The linked shared-channel slow-hook incident has no committed end-to-end replaypackages/acp-bridge/src/bridge.test.ts:12800 — [probe] The deadline metadata test accepts any numeric valuepackages/acp-bridge/src/bridge.test.ts:13332 — [review] The per-request overdue-state test never makes both requests overdue simultaneouslypackages/acp-bridge/src/bridge.test.ts:13347 — [review] Capacity tests do not cover pending abandonment when spare capacity remainspackages/acp-bridge/src/bridge.ts:13612 — [review] Shutdown waiting for abandoned newSession settlements is not regression-testedpackages/acp-bridge/src/bridgeErrors.ts:638 — [review] BridgeChannelQuarantinedError JSDoc understates late-success cleanup requirementspackages/cli/src/acp-integration/acpAgent.test.ts:2314 — [review] Agent tests do not pin checkpoints after late-resolving non-cancellable phasespackages/cli/src/acp-integration/acpAgent.test.ts:2362 — [review] The maximum-deadline boundary test has only millisecond-scale race slackpackages/cli/src/serve/server/error-response.test.ts:77 — [review] The REST route does not exercise BridgeTimeoutError mapping end to endpackages/core/src/config/config.test.ts:4377 — [review] Config tests do not isolate checkpoints after late-resolving initialization phasespackages/core/src/core/client.ts:520 — [review] Restore branches do not have focused tests for AbortSignal forwarding
Convergence: round 6 posted 1 inline comment(s), 1 of them reported for the first time. Findings keep coming back to the same files: packages/cli/src/serve/server/error-response.ts (findings in round 5; 1 more now). (Evidence: the previous round was recovered from a marker this account did not post and merged over this account's own entries, so some of those rounds may not be this account's own.) 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.)
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.)
[Critical] R6-2 [certifies-falsely] [new-surface] Abandoned restore cleanup accepts a close refusal as successful cleanup at packages/acp-bridge/src/bridge.ts:7488. A timed-out restore that later succeeds can receive {closed:false} from sessionClose; the restore path ignores that response, records cleanup as closed, clears the fence, and reopens admission while the late Session may remain alive. Validate closed === true exactly as the abandoned newSession cleanup path does. Existing sibling sessions must remain usable during quarantine and channel drain, as documented in docs/developers/qwen-serve-protocol.md:2175. Add a bridge test where late restore cleanup returns closed:false; removing the validation must make the test fail. Witness: not run — the fulfilled-response path deterministically never inspects closed.
[Critical] packages/core/src/hooks/instructionsLoadedCallback.ts:29 — [review] InstructionsLoaded hooks do not receive the initialization deadline signal (relocated from the deferral channel — a Critical is never deferred, it posts)
[Critical] packages/acp-bridge/src/bridge.ts:7569 — [probe] Restore settlement-overdue remains set after the only overdue restore settles (relocated from the deferral channel — a Critical is never deferred, it posts)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse-audit chunk 5 round 4 — auditor failed twice; round 5 returned a substantive dry receipt。
未审查：reverse-audit chunk 7 round 3 — receipt wording did not satisfy retirement certification; rounds 4 and 5 returned substantive dry receipts。
未审查："agent verify (round 2)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 12 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 1 条行内评论，其中 1 条是首次提出。发现反复回到同一批文件：packages/cli/src/serve/server/error-response.ts（第 5 轮已出过发现，本轮又有 1 条）。（证据说明：上一轮的数据来自并非本账号发布的标记，并与本账号自己的条目合并，上述轮次中的部分可能不属于本账号。）一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff，除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上；并会一直如此，直到某一轮的标记重新带上锚点，或落地的嫁接能被运行该轮的评审使用。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
[Critical] R6-2 [certifies-falsely] [new-surface] Abandoned restore cleanup accepts a close refusal as successful cleanup at packages/acp-bridge/src/bridge.ts:7488. A timed-out restore that later succeeds can receive {closed:false} from sessionClose; the restore path ignores that response, records cleanup as closed, clears the fence, and reopens admission while the late Session may remain alive. Validate closed === true exactly as the abandoned newSession cleanup path does. Existing sibling sessions must remain usable during quarantine and channel drain, as documented in docs/developers/qwen-serve-protocol.md:2175. Add a bridge test where late restore cleanup returns closed:false; removing the validation must make the test fail. Witness: not run — the fulfilled-response path deterministically never inspects closed.
[Critical] packages/core/src/hooks/instructionsLoadedCallback.ts:29 — [review] InstructionsLoaded hooks do not receive the initialization deadline signal (relocated from the deferral channel — a Critical is never deferred, it posts)
[Critical] packages/acp-bridge/src/bridge.ts:7569 — [probe] Restore settlement-overdue remains set after the only overdue restore settles (relocated from the deferral channel — a Critical is never deferred, it posts)
— gpt-5.6-sol@954e5164 via Qwen Code /review (v0.22.2)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed all four round-6 Critical findings in 93c909c.
Verification: CLI mapper tests 46/46, ACP Bridge 822/822, Core Config and callback 594/594, HookRunner cancellation checks 3/3, relevant ESLint, Prettier, diff checks, and ACP Bridge/Core typechecks passed. The repository-wide build/typecheck remains blocked only by the known unchanged Ink selection API/type mismatch in CLI UI code; no changed-file diagnostic was emitted. One inline thread was replied to and is being resolved. |
6 participants
What this PR does
This PR makes the existing daemon session initialization budget authoritative end to end. The bridge sends a private absolute deadline with each managed ACP new-session request, the child propagates cancellation through configuration, Gemini startup, and
SessionStarthooks, and a timed-out Session is rejected before publication. For older children that ignore the deadline, the bridge tracks the late request, closes any late-created Session by exact ID, retains admission and caller-supplied ID fences until cleanup settles, and quarantines only fresh session creation when cleanup remains uncertain. Healthy sibling Sessions on a shared channel remain usable.Why it's needed
The public
newSessiontimeout previously rejected only the bridge wrapper while the underlying child work could continue. A slowSessionStartcommand hook could therefore finish after the caller receivedinit_timeout, leave descendant processes running, and publish a real child Session that the bridge never registered. Killing the shared ACP channel at the first timeout would avoid hidden work but would also terminate unrelated healthy Sessions, so the timeout needs request-scoped cancellation plus a bounded compatibility lifecycle for older children.Reviewer Test Plan
How to verify
On a managed shared ACP channel, keep one healthy Session alive and create a second Session whose
SessionStartcommand hook exceeds the initialization budget. Expect the second request to fail with the existinginit_timeoutcontract, never publish a Session, and reclaim both the hook root and descendant, while the healthy sibling still completes a prompt. With a compatibility child that ignores the private deadline, resolvenewSessionafter the public timeout and confirm the bridge sends exactly one close request for the returned Session ID. Make the close returnclosed: false, and separately leave the raw request unsettled for one further budget; in both cases expect fresh admission to fail with the documentedacp_channel_unavailablereason while the sibling remains usable.Evidence (Before & After)
Before: the caller received
init_timeout, but an older or non-cancelling child could finish initialization later, publish an unregistered Session, and leave aSessionStarthook process tree running.After: managed children abort before Session publication and tree-aware Hook cancellation reclaims the root and descendant. Older children are contained by exact-ID late cleanup; uncertain cleanup fails closed only for fresh admission and does not terminate healthy siblings.
Tested on
Environment (optional)
macOS 26.4.1 (Darwin 25.4.0 arm64), Node.js 22.22.3, npm 10.9.8. The full ACP Bridge test file passed 809 tests, the ACP Agent test file passed 511 tests, and the Core client plus HookRunner files passed 424 tests. The affected ACP Bridge and Core packages passed build and typecheck; changed files passed ESLint, Prettier, and
git diff --check. A real HookRunner process-tree harness confirmed that an abort reaped both the hook root and a descendant that ignored SIGTERM. The repository-widenpm run build && npm run bundleremains blocked before bundling by the current main branch's unrelated Ink selection API/type mismatch in CLI UI files.Risk & Scope
init_timeout; the private deadline is restricted to the trusted managed-parent path. Two additiveacp_channel_unavailablereason values are documented for clients that inspect quarantine details.Linked Issues
Closes #10266
中文说明
本 PR 做了什么
本 PR 让现有 daemon Session 初始化预算在整条链路上具备权威性。Bridge 会在每个受管 ACP 新建 Session 请求中发送私有的绝对 deadline，子进程将取消信号传递到配置初始化、Gemini 启动和
SessionStartHook，并在发布前拒绝已经超时的 Session。对于忽略 deadline 的旧子进程，Bridge 会跟踪迟到请求，按精确 Session ID 关闭迟到创建的 Session，在清理完成前保留准入和调用方指定 ID 的 fence，并在无法确认清理结果时只隔离新的 Session 创建。共享 channel 上的健康兄弟 Session 仍然可用。为什么需要
此前公开的
newSession超时只会拒绝 Bridge 包装层，底层子进程工作仍可能继续。缓慢的SessionStart命令 Hook 可能在调用方已经收到init_timeout后才完成，留下仍在运行的后代进程，并发布一个 Bridge 从未注册的真实子 Session。首次超时时直接终止共享 ACP channel 虽然可以避免隐藏任务，但也会终止无关的健康 Session，因此这里需要请求级取消，以及面向旧子进程的有界兼容生命周期。Reviewer 测试计划
如何验证
在受管共享 ACP channel 上保留一个健康 Session，并创建第二个 Session，使它的
SessionStart命令 Hook 超过初始化预算。预期第二个请求以现有init_timeout契约失败、始终不发布 Session，并回收 Hook 根进程和后代进程，同时健康兄弟仍可完成一次 prompt。再使用忽略私有 deadline 的兼容子进程，在公开超时后让newSession返回，并确认 Bridge 仅针对返回的 Session ID 发送一次精确 close 请求。让 close 返回closed: false，并单独让原始请求再经过一个完整预算仍不 settle；两种情况下都应看到新准入以文档化的acp_channel_unavailablereason 失败，而兄弟 Session 仍然可用。证据（前后对比）
改动前：调用方会收到
init_timeout，但旧版或无法取消的子进程仍可能稍后完成初始化，发布一个未注册的 Session，并留下仍在运行的SessionStartHook 进程树。改动后：受管子进程会在 Session 发布前中止，具备进程树感知能力的 Hook 取消会回收根进程和后代。旧子进程通过精确 ID 的迟到清理进行约束；清理结果不确定时只对新准入失败关闭，不会终止健康兄弟。
测试平台
环境（可选）
macOS 26.4.1（Darwin 25.4.0 arm64）、Node.js 22.22.3、npm 10.9.8。完整 ACP Bridge 测试文件通过 809 个测试，ACP Agent 测试文件通过 511 个测试，Core client 与 HookRunner 文件通过 424 个测试。受影响的 ACP Bridge 和 Core 包通过 build 与 typecheck；改动文件通过 ESLint、Prettier 和
git diff --check。真实 HookRunner 进程树测试确认 abort 会回收 Hook 根进程，以及一个忽略 SIGTERM 的后代进程。全仓库npm run build && npm run bundle仍在 bundle 前被当前 main 分支中与本改动无关的 CLI UI Ink selection API/类型不匹配阻断。风险与范围
init_timeout；私有 deadline 仅限可信受管父进程路径。针对检查隔离细节的客户端，文档新增了两个附加的acp_channel_unavailablereason 值。关联 Issue
Closes #10266