fix(daemon): Bound conditional-close refusal holds - #9820
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ Re-run note: two things landed since the last gate pass — a feedback commit adding the regression test the review asked for (test-only), and a merge of Problem: Real, and I re-verified it in the current merge base rather than trusting the prior pass: Direction: Aligned — same active-work deep-health effort (#8586), tightening a boundary that effort introduced. No public-contract, auth, or sandbox surface touched: private daemon-to-child protocol, lifecycle retention, tests, and docs only. Size: Not a core-module path ( Approach: Still minimal. The fix is unchanged from the prior pass and remains the obvious correct one — apply the existing constant before the walk, refuse adoption whole, keep the last valid cache, fail closed on retention. The new commit adds exactly the test the review asked for: a stale non-empty cache must survive an oversized refusal rather than being replaced. The follow-up also declines the "log oversized refusals" suggestion with a reasoned argument (observability-only; safe rate limiting would need per-entry lifecycle state) — a reasonable scoping call, not a gap. Risk: No elevated risk signals — none of the changed paths are in the revert-correlated set. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ Re-run 说明： 自上次通过门禁以来有两件事——按 review 要求补上回归测试的反馈提交（仅测试），以及合并 问题： 真实存在，且我在当前合并基线上重新核实过，而非沿用上次结论： 方向： 对齐——同属 #8586 的 active-work deep-health 工作，收紧的是该功能引入的边界。不涉及公开契约、auth 或 sandbox：仅私有 daemon-子进程协议、生命周期保留、测试和文档。 规模： 非核心模块路径（ 方案： 仍然是最小改动。修复与上次一致，依旧是显而易见的正确做法——在遍历前应用已有常量、整体拒绝采纳、保留最后一次有效缓存、在保留语义上 fail-closed。新提交恰好补上了 review 要求的测试：陈旧的非空缓存在超大拒绝后必须保留而不是被替换。跟进说明还以合理理由拒绝了"记录超大拒绝日志"的建议（纯可观测性；安全的限流需要 per-entry 生命周期状态）——这是合理的范围取舍，不是缺口。 风险： 无升级风险信号——改动路径均不在与 revert 相关的高风险集合中。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review. The production diff is byte-identical to what the previous pass reviewed — the only change since is the feedback commit, one test-only addition (+43 in The new test is the interesting part, and it is the right one. It seeds a non-empty snapshot cache, ages it past the staleness threshold with fake timers, then refuses the conditional close with 1,025 holds. On the old code the oversized list would have been adopted — Testing. Evidence carried here: the PR's own CI check results and job log for this commit, fetched via the API, plus static review. This is an unattended CI run — no PR code was built or executed by this review. The one reservation from the prior run is resolved. The earlier red
The author-triggered sandboxed run is already in flight: 中文说明代码审查。 生产代码 diff 与上次审查完全一致——此后唯一的变化是反馈提交，一处纯测试新增（ 新测试是重点，而且加得对。它先种下一份非空快照缓存，用假定时器把它推过过期阈值，再让 conditional close 以 1,025 条 holds 拒绝。旧代码会采纳超大列表—— 测试。 本节携带的证据：通过 API 获取的本提交自身 CI 检查结果与作业日志，外加静态审查。这是无人值守的 CI 运行，本审查未构建或执行任何 PR 代码。 上次运行唯一的保留意见已消除。此前红色的 作者触发的沙箱验证已在运行中： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — every reservation from the prior pass is now closed: the review feedback was answered with a regression test that pins exactly the right semantics, and this commit's unit suite actually ran in CI and came back green. Stepping back: the previous pass approved at 4/5 with one named reservation — the unit suite had never executed on the reviewed commit because a pre-existing workflow-size precheck on If I had to maintain this in six months: two bounded intake sites agreeing on one named constant, a doc that matches the code, and tests on both sides of the boundary plus the stale-cache edge. Nothing to curse. Approving, pinned to the reviewed commit. The author-triggered 中文说明整体来看：上一轮以 4/5 批准时留有一个明确的保留意见——由于 如果六个月后由我来维护：两个受限入口共用一个具名常量、文档与代码一致、边界两侧加陈旧缓存边角都有测试。无可指摘。现予以批准，并固定到所审查的提交。作者触发的 — 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.
LGTM, looks ready to ship. ✅
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.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 silent oversized-refusal discard lacks a log line — already reported (comment 3839158717)
中文说明
已审查。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
[codex] Review follow-up for
Validation: bridge tests 778/778 passed; ACP bridge build and typecheck passed; targeted lint, format, and diff checks passed. The test engineer independently reproduced the review gap and verified the new test plus the 1,024/1,025 boundary cases (3/3). CI note: the prior Ubuntu failure was the unrelated workflow-size precheck for |
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.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(bridge.sessionCount).toBe(1); | ||
| expect(bridge.activeWork).toBe(true); | ||
| expect(reportingGrade(bridge)).toBe('partial'); |
There was a problem hiding this comment.
[Suggestion] R2-1: The new 'retains a stale non-empty cache after an oversized close refusal' test asserts only the post-detach state — session retained, activeWork true, grade 'partial' — and never asserts that the conditional-close round trip actually happened. An oversized refusal mutates nothing observable in confirmChildUnheld (no cache update, no log line — only the timeout branch logs), so the asserted state is byte-identical whether the child was asked or not. A mutation probe confirmed the hole: making entryIsAutoCloseCandidate skip the probe when work is unknown — !childReportsHeldWork(entry) && !childWorkIsUnknown(entry), the exact anti-pattern the gate's design comment forbids ("Skipping on unknown instead would retain such a Session forever") — leaves this test green without any ext call being made, while adding the assertion below flips it to failing (and it stays green against the real code). The helper already records the evidence (handle.agent.extMethodCalls), and this file already asserts on recorded close params elsewhere (~line 644).
| expect(bridge.sessionCount).toBe(1); | |
| expect(bridge.activeWork).toBe(true); | |
| expect(reportingGrade(bridge)).toBe('partial'); | |
| expect(bridge.sessionCount).toBe(1); | |
| expect(bridge.activeWork).toBe(true); | |
| expect(reportingGrade(bridge)).toBe('partial'); | |
| expect( | |
| handle.agent.extMethodCalls.some( | |
| (call) => | |
| call.method === SERVE_CONTROL_EXT_METHODS.sessionClose && | |
| call.params?.[ACTIVE_WORK_CLOSE_IF_UNHELD_PARAM] === true, | |
| ), | |
| ).toBe(true); |
中文说明
新增的 'retains a stale non-empty cache after an oversized close refusal' 测试只断言了 detach 之后的状态——Session 保留、activeWork 为 true、grading 为 'partial'——但从未断言 conditional-close 往返确实发生过。超大拒绝在 confirmChildUnheld 中不会产生任何可观察的变化（不更新缓存、不打日志——只有超时分支会打日志），因此无论是否询问了 child，断言的状态都完全相同。变异探测证实了这个缺口：让 entryIsAutoCloseCandidate 在 work 未知时跳过探测——即 !childReportsHeldWork(entry) && !childWorkIsUnknown(entry)，正是该门控设计注释明确禁止的反模式（"Skipping on unknown instead would retain such a Session forever"）——在没有发生任何 ext 调用的情况下测试仍然通过；而补充上面这条断言后，该变异即被捕获（对真实代码则保持通过）。测试辅助工具已经记录了所需证据（handle.agent.extMethodCalls），且本文件其他位置（约 644 行）已有对已记录 close 参数进行断言的先例。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- oversized-refusal logging on the silent branch (round-3 audit candidate RA2-B) — already reported as R1-1 (comment 3839158717) and declined by the author in reply 3839186549
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect(bridge.sessionCount).toBe(1); | ||
| expect(bridge.activeWork).toBe(true); | ||
| expect(reportingGrade(bridge)).toBe('partial'); |
There was a problem hiding this comment.
[Suggestion] R2-1: The 'retains a stale non-empty cache after an oversized close refusal' test still asserts only the post-detach state — session retained, activeWork true, grade 'partial' — and nothing in it proves the conditional-close round trip actually ran. An oversized refusal changes nothing observable in confirmChildUnheld (no cache update, no log line), so the asserted state is byte-identical whether the child was asked or not. Concretely: if entryIsAutoCloseCandidate were mutated to skip the probe when work is unknown — the exact anti-pattern the gate's design comment forbids — no sessionClose call would ever be made and this test would still pass. Re-verified this round with a scratch-tree probe at d21216c: the mutant leaves the test green, and adding the assertion below flips the mutant to failing while staying green on the real code.
| expect(bridge.sessionCount).toBe(1); | |
| expect(bridge.activeWork).toBe(true); | |
| expect(reportingGrade(bridge)).toBe('partial'); | |
| expect(bridge.sessionCount).toBe(1); | |
| expect(bridge.activeWork).toBe(true); | |
| expect(reportingGrade(bridge)).toBe('partial'); | |
| expect( | |
| handle.agent.extMethodCalls.some( | |
| (call) => | |
| call.method === SERVE_CONTROL_EXT_METHODS.sessionClose && | |
| call.params?.[ACTIVE_WORK_CLOSE_IF_UNHELD_PARAM] === true, | |
| ), | |
| ).toBe(true); |
中文说明
'retains a stale non-empty cache after an oversized close refusal' 测试仍然只断言了 detach 之后的状态——Session 保留、activeWork 为 true、grading 为 'partial'——其中没有任何断言能证明 conditional-close 往返确实发生过。超大拒绝在 confirmChildUnheld 中不会产生任何可观察的变化（不更新缓存、不打日志），因此无论是否询问了 child，断言的状态都完全相同。具体来说：如果将 entryIsAutoCloseCandidate 变异为在 work 未知时跳过探测——正是该门控设计注释明确禁止的反模式——sessionClose 调用根本不会发生，而这个测试依然会通过。本轮已在 d21216c 的 scratch-tree 中重新验证：该变异下测试仍然为绿；补充下方断言后，变异即被捕获（对真实代码保持通过）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
@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: 1729 passed · 0 failed · 1729 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1729 通过 · 0 失败 · 1729 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9820 Deep Verification — fix(daemon): Bound conditional-close refusal holdsVerdict: 中文摘要结论：
Scope
Code under test: one guard in Central claim — A/B load-bearing proofMethod: base worktree at
15 oracle checks per arm, all matching the arm's encoded expectation (the base-arm B/C rows are the predicted-bug control cells; base failing the same way the issue describes is what makes the A/B load-bearing). Witness: Cell C is the sharp one for the PR's second promise ("does not replace the last valid cache"): with the clock advanced past Scale sibling ( Boundary consistency with the sibling bound: snapshot ingestion rejects at Vacuity check / mutation matrixThe PR introduces one guard. Mutation: restore the exact base form (
Both reds are the intended behavioral assertions (expected-vs-actual values quoted above), not import/compile/fixture breakage. Positive control: the identical command with the guard present (i.e. the head tree) is green — the same file, same filter turns red only when the guard is gone, which also proves the test gate itself is live. No combination row applies (single-hunk defence; the PR has no second guard). The tests' names match their fixtures (counts are FindingsNone. Probed and clean: at-limit/over-limit boundary, non-array and missing-holds refusals, success path, fail-closed retention ( Targeted gates
Doc claims vs code (secondary claim 2)All three verified against the implementation:
Not covered
MethodologyEnvironment: CI verify container ( 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.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified at head d21216c:
- The fix is exactly the bound the body describes: the refused hold set is adopted only when
Array.isArray(holds) && holds.length <= ACTIVE_WORK_MAX_SESSION_HOLDS— the same 1,024 per-Session bound the snapshot bounds already use. An oversized refusal skips adoption entirely, sochildHolds/childHoldsAtkeep the last valid cache instead of walking and adopting the oversized list, while the refusal still returns false and the Session stays registered — fail-closed retention is untouched. - The in-bound adoption path keeps its per-entry validation (id/category shape + ACTIVE_WORK_HOLD_CATEGORIES membership), and the constant's doc now states both behaviors (oversized snapshots discarded whole; oversized close refusals retain without replacing the cache).
- Tests pin both sides of the bound at exactly 1,024 vs 1,025 for the snapshot path with reporting-grade assertions, plus the oversized-refusal retention case.
- The design-doc alignment (response shape, trigger families, daemon-wide grading) matches the implemented behavior as described.
One non-blocking note: the bot's R2-1 thread on the oversized-refusal test is a fair mutation-sensitivity observation — since an oversized refusal mutates nothing observable, the retained-state assertions can't distinguish "the child was asked and refused oversized" from "the probe never ran". That is test-hardening rather than a defect in this fix (the bound guard itself is a pure function of holds.length, and the snapshot-side parametrized cases pin the bound logic), so I leave it to the author whether to add a round-trip witness (e.g., a spy on the ext-method call or a log line) in a follow-up.
CI at approval time: 20 checks passing, none failing.
|
Released in v0.22.2. |
4 participants
What this PR does
This PR bounds conditional-close refusal hold adoption to the same 1,024-hold per-Session limit used by active-work snapshots. A response at the limit is still adopted; an oversized response still refuses the close and retains the Session, but leaves the last valid cache intact instead of walking and adopting the oversized list.
It also aligns the active-work design documentation with the implemented conditional-close response shape, all six automatic-cleanup trigger families, and daemon-wide coverage grading after counts are aggregated across runtimes.
Why it's needed
Conditional-close refusals cross the private ACP child boundary. Before this change, the daemon traversed and adopted any returned array even though snapshot ingestion already rejected more than 1,024 holds. A 1,025-hold refusal therefore retained the Session but replaced a fresh empty cache and changed
activeWorkfrom false to true. Applying the existing bound before traversal prevents unbounded follow-up work and prevents an oversized refusal from refreshing active-work evidence, without weakening fail-closed Session retention.The related issue and design documents had also drifted after the shell extension and lifecycle hardening landed, leaving the documented scope, success response, trigger count, and cross-runtime grade inconsistent with current behavior.
Reviewer Test Plan
How to verify
Use a negotiated ACP child with a fresh empty active-work snapshot, detach the last client, and have conditional close refuse with valid holds. With exactly 1,024 holds, confirm the Session remains registered, the returned holds are adopted,
activeWorkis true, and reporting remains full. With 1,025 holds, confirm the Session still remains registered, the oversized list is not adopted,activeWorkremains false from the last valid cache, and reporting remains full.Confirm the ordinary in-bound refusal path and oversized snapshot rejection still behave unchanged. Review the design text against the child response shape, the detach/prompt/notification/snapshot/attach-rollback/reaper triggers, and the daemon-wide coverage aggregation.
Evidence (Before & After)
N/A — private daemon protocol, lifecycle retention, tests, and documentation only; there is no user-visible or TUI change.
Tested on
Environment (optional)
macOS 26.4.1, Node.js 22.22.3, npm 10.9.8, package-level Vitest, TypeScript, ESLint, and Prettier without sandboxing.
Risk & Scope
Linked Issues
Refs #8586
中文说明
本 PR 做了什么
本 PR 将 conditional-close 拒绝响应的 hold 采纳限制为 active-work 快照已经使用的每 Session 1,024 条上限。恰好达到上限的响应仍会被采纳；超出上限的响应仍然拒绝关闭并保留 Session，但不会遍历和采纳超大列表，而是保留最后一次有效缓存。
同时更新 active-work 设计文档，使其与已经实现的 conditional-close 响应结构、全部六类自动清理触发事件，以及跨 runtime 汇总 coverage 计数后再进行 daemon 级分级的行为保持一致。
为什么需要
Conditional-close 拒绝响应来自私有 ACP 子进程边界。变更前，快照入口已经拒绝超过 1,024 条的 holds，但 daemon 仍会遍历并采纳拒绝响应中的任意数组。返回 1,025 条 holds 时，Session 虽然会保留，但新鲜的空缓存会被替换，
activeWork也会从 false 变为 true。在遍历前应用已有上限，可以避免无界的后续处理，并防止超大拒绝响应刷新 active-work 证据，同时不削弱 fail-closed 的 Session 保留语义。在 shell 范围扩展和生命周期加固合并后，相关 issue 与设计文档也出现了漂移，导致记录的范围、成功响应、触发器数量和跨 runtime 分级与当前行为不一致。
Reviewer 测试计划
验证方式
使用已经协商 active-work 的 ACP 子进程，先发送新鲜的空快照，再 detach 最后一个客户端，并让 conditional close 返回有效 holds 以拒绝关闭。恰好 1,024 条时，确认 Session 仍然注册、返回的 holds 被采纳、
activeWork为 true 且 reporting 保持 full。返回 1,025 条时，确认 Session 同样保留、超大列表不被采纳、activeWork维持最后一次有效缓存中的 false，且 reporting 保持 full。同时确认普通的界内拒绝路径和超大快照拒绝行为保持不变。将设计文本与 child 响应结构、detach/prompt/notification/snapshot/attach rollback/reaper 六类触发事件，以及 daemon 级 coverage 汇总实现进行核对。
证据（Before & After）
N/A —— 仅涉及私有 daemon 协议、生命周期保留、测试和文档，不包含用户可见或 TUI 变更。
测试平台
环境（可选）
macOS 26.4.1、Node.js 22.22.3、npm 10.9.8，未启用 sandbox，运行 package 级 Vitest、TypeScript、ESLint 和 Prettier。
风险与范围
关联 Issue
Refs #8586