feat(daemon): Track background shells in activeWork - #9042
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E reportBaselineOn macOS with the released global AfterThe same flow against the local build kept the running shell visible and returned Automated verification
The full repository build/typecheck could not complete in this checkout because the local installed Ink package rejected the repository's existing |
|
Thanks for the PR — this is the documented background-shell follow-up to the Template looks good ✓ Problem: observed, not theoretical. Layer-1 Direction: aligned. This is the extension path the umbrella issue explicitly designed for — holds carry a category so the scope widens by adding data rather than changing what the boolean means — and the widening is documented in the #8586 thread by the issue author. The public health shape and protocol version stay unchanged; compatibility is negotiated per channel, not assumed. Size: core paths touched ( Approach: the scope feels right. One bounded aggregate hold instead of a per-shell roster keeps the protocol inside its 1024-holds cap; filtering happens only at wire serialization so the local conditional-close check stays truthful against an older daemon; re-reading the collector after active turns drain closes the window where an already-running cron turn registers a shell mid-teardown. I didn't spot unrelated changes or drive-by edits. What the re-run does flag is the ordering inside that conditional-close path — the drain's destructive steps run before the read that can refuse the close — which is a Stage 2 finding, not a gate objection. Risk: Moving on to code review. 🔍 中文说明感谢贡献——这是 模板完整 ✓ 问题：已观测到，不是理论问题。Layer-1 的 方向：对齐。这正是伞形 issue 明确预留的扩展路径——hold 携带 category，范围通过增加数据而非改变布尔语义来扩展——且这次范围扩展已由 issue 作者记录在 #8586 讨论中。公开 health 结构和协议版本不变；兼容性按 channel 协商，而不是默认假设。 规模：触及核心路径（ 方案：范围合理。用一个有界的聚合 hold 代替逐 shell 清单，让协议保持在 1024 hold 上限内；过滤只发生在 wire 序列化处，使本地 conditional-close 检查在面对旧 daemon 时仍然真实；在 active turn 排空后重读 collector，关闭了已运行的 cron turn 在 teardown 期间注册 shell 的窗口。未发现无关改动或顺手修改。重跑真正标记的是该 conditional-close 路径内部的顺序问题——drain 的破坏性步骤跑在了可以拒绝关闭的读取之前——这是 Stage 2 的发现，不是门禁异议。 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review (re-run)This pass re-reviewed the code with the deep Blocking — the reject-after-drain outcome can retain a Session whose queued work the drain already destroyed. The ordering in Deferred spawn-owner kills are silently suspended for incomplete children. The finding's four suggestions also check out statically, and they're worth taking in the same revision: the "covers every required category" predicate is now hand-inlined twice (cleanup guard bridge.ts:2244-2245 vs health grading bridge.ts:6750-6751 — extract one into What the re-run re-confirmed as solid (unchanged from the first pass): the derived aggregate hold with no ledger to leak, wire-only filtering with an unfiltered local close check, the fail-closed compatibility matrix in every pairing, identity-safe callback retraction, and the queued-notification → continuation handoff. The design is right; the conditional-close ordering is what needs fixing. sequenceDiagram
participant P1 as Prompt turn
participant P2 as Shell registry
participant P3 as Session hold collector
participant P4 as Active-work reporter
participant P5 as Daemon bridge
P1->>P2: register background shell
P2->>P3: status change, running
P3->>P4: notifyChanged
P4->>P5: snapshot with shell hold
Note over P3: hold true while running or queued or in continuation
P2->>P3: completion notification queued first
P2->>P3: then entry flips to terminal
P3->>P4: hold stays true via the queue term
P3->>P3: drain marks continuation active
P3->>P4: hold released only after the continuation settles
Files changed (16 of 16 shown)
TestingUnattended CI run — no PR code was executed here; the evidence below is the PR's own CI read through the API, plus verification reports posted in this thread and attributed to their authors. All checks on the reviewed commit have completed — nothing pending, nothing red. The Linux full profile (fork PR) is green: Two independent real-stack reports are on record, attributed to their authors (not re-run here): the maintainer's A/B (
Sandboxed verification would settle what's still open after the fix: 中文说明代码审查（重跑）本次带着深度 阻塞项——drain 后拒绝关闭的结果可能保留一个其排队工作已被 drain 摧毁的 Session。 对类别不完整的 child，延迟的 spawn-owner kill 被静默挂起。 四个 Suggestion 经静态核对也都成立，值得在同一轮修订中一并处理："覆盖全部必需类别"谓词现已手工内联两次（清理保护 bridge.ts:2244-2245 vs 健康分级 bridge.ts:6750-6751——抽取到 重跑再次确认仍然扎实的部分（与首轮一致）：派生的聚合 hold、没有可泄漏的台账、只在 wire 处过滤而本地关闭检查保持未过滤、所有配对下 fail-closed 的兼容矩阵、身份安全的回调撤回、排队通知 → continuation 的交接。设计是对的；需要修的是 conditional-close 的顺序。 测试无人值守 CI 运行——此处未执行任何 PR 代码；以下证据是通过 API 读取的 PR 自身 CI，以及本线程中署名作者的验证报告。 被审提交上的所有 check 均已完成——无 pending、无红灯。Linux 全 profile（fork PR）全绿： 两份独立的真实栈报告记录在案，归属于其作者（未在此重跑）：维护者的 A/B（ 沙箱验证可以补上修复后仍开放的缺口： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the design is genuinely good and the maintainer's real-stack A/B settled every behavioural claim the PR makes; but the re-run confirmed a blocking defect those runs didn't reach — the new reject-after-drain path can retain a Session whose queued work the drain has already destroyed — so I can't stand behind approval as-is. Stepping back: my independent proposal before reading the diff was the same shape the PR implements — one derived aggregate hold, wire-only filtering, negotiated categories, fail-closed retention — and I still think that shape is right; the PR's version is better than mine at the edges I did check. What I missed in the first pass, and what this re-run verified line by line, is that the conditional-close fix is ordered destructively: To @wenshao — your approval is on record and your A/B is the strongest evidence this PR has; this disagrees with it on one narrow scenario that run didn't exercise (a cron turn registering a shell while a conditional close drains it), and the Stage 2 comment has the full trace if the code reads differently to you than it did to me. To @doudouOUC — this is close; the retention design itself is sound and independently verified, so the ask is the conditional-close ordering plus the tombstone branch, with regression tests that pin both, and the four suggestions are worth sweeping in the same round. Housekeeping: a 中文说明信心：2/5 —— 设计确实出色，维护者的真实栈 A/B 也已经钉住了 PR 所做的全部行为声称；但重跑确认了一个那些运行未触及的阻塞缺陷——新的 drain 后拒绝路径可能保留一个其排队工作已被 drain 摧毁的 Session——因此我无法为现状背书批准。 退一步看：我在读 diff 之前的独立方案与 PR 实现的形状相同——一个派生的聚合 hold、只在 wire 处过滤、协商类别、fail-closed 保留——我仍然认为这个形状是对的；PR 的版本在我核对过的边界上比我的更好。首轮我漏掉、本轮逐行核实的是：conditional-close 修复的破坏性步骤排在了可以拒绝关闭的读取之前—— @wenshao —— 你的批准记录在案，你的 A/B 是本 PR 最有力的证据；本结论与之分歧于该运行未演练的一个窄场景（cron turn 在 conditional close drain 期间注册 shell），如果代码在你看来与我的结论不同，Stage 2 评论里有完整的逐行追踪。@doudouOUC —— 这个 PR 离合并很近；保留设计本身是扎实且已被独立验证的，因此请求是修正 conditional-close 顺序与 tombstone 分支，补上钉住两者的回归测试，四个 Suggestion 值得在同一轮一并处理。 事务性说明：本 bot 账号在 — 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.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — no check was left unfinished.; "This PR extends the Session activeWork retention fact to…": None — no checks were left unfinished.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "This PR extends the Session activeWork retention fact to…": none — all checks I started completed within budget.; "This PR extends the Session activeWork retention fact to…": none — all checks I started were completed within budget., and 1 more.
中文说明
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — no check was left unfinished.；"This PR extends the Session activeWork retention fact to…"：None — no checks were left unfinished.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"This PR extends the Session activeWork retention fact to…"：none — all checks I started completed within budget.；"This PR extends the Session activeWork retention fact to…"：none — all checks I started were completed within budget.，另有 1 条。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // Existing out-of-scope work such as a cron turn may have registered a | ||
| // background shell while it drained. Re-check after every active turn | ||
| // has settled and while the close gate still blocks new ones; only this | ||
| // read can authorize the destructive recorder/session cleanup below. | ||
| if (opts?.onlyIfUnheld) { | ||
| const holds = session.collectActiveWorkHolds(); |
There was a problem hiding this comment.
[Critical] The new post-drain rejection can retain a Session after the close probe has already destroyed its queued work — the drain is destructive even when it refuses to close. Before this second read runs, the drain's session.cancelPendingPrompt() (Session.ts:3352-3432) aborts the running turn, clears cronQueue, splices and aborts queued goal turns, clears notificationQueue, suspends the todo stop guard, and unconditionally calls #stopCronSchedulerInRuntime().
Failure scenario: a detached Session runs an out-of-scope cron turn (invisible to activeWork by design) → conditional close passes the early read → the drain runs cancelPendingPrompt() → the aborting turn registers a background shell just before the abort lands (the exact race this second read exists for) → this read finds the shell hold and returns closed: false, so the Session is retained — but its cron scheduler is stopped (the only restart sites are Session creation and prompt()'s finally; cron turns run via #executeCronPromptInner, so a detached retained session never restarts it), its queued cron/@wakeup/goal items are permanently gone, and nothing logs any of it. Pre-PR this destruction was always followed by teardown, so it was harmless; the new reject-after-drain outcome is what makes it loss. The drain-timeout variant reaches the same destroyed-but-retained state.
Suggested fix: on the onlyIfUnheld path, defer the destructive steps until after authorization — wait for natural settlement (waitForActiveTurnsToSettle() bounded by drainTimeoutMs) before this read, and run the generation-controller aborts and cancelPendingPrompt() only once both reads came back empty (right before recorder?.finalize()); alternatively, restore the cron scheduler and re-arm the cleared queue state on the rejection path.
中文说明
新的 drain 后复查拒绝路径可能在关闭探测已经摧毁 Session 排队工作之后才保留该 Session —— 即使最终拒绝关闭，drain 本身也是破坏性的。在这次第二次读取运行之前，drain 中的 session.cancelPendingPrompt()（Session.ts:3352-3432）会中止正在运行的 turn、清空 cronQueue、splice 并中止排队的 goal turns、清空 notificationQueue、挂起 todo stop guard，并无条件调用 #stopCronSchedulerInRuntime()。
失败场景：一个 detached Session 正在运行范围外的 cron turn（按设计对 activeWork 不可见）→ conditional close 通过了提前读取 → drain 运行 cancelPendingPrompt() → 被中止的 turn 在中止落地前一刻登记了一个后台 shell（正是这第二次读取要防的竞态）→ 本次读取发现 shell hold 并返回 closed: false，Session 被保留 —— 但其 cron scheduler 已被停止（重启点只有 Session 创建和 prompt() 的 finally；cron turn 通过 #executeCronPromptInner 执行，因此 detached 且被保留的 session 永远不会重启它），排队的 cron/@wakeup/goal 项永久丢失，且没有任何日志。此 PR 之前，这种破坏之后总是紧跟 teardown，因此无害；新的“先破坏、后拒绝”结果才使其成为损失。drain 超时变体同样到达“被破坏但被保留”的状态。
建议修复：在 onlyIfUnheld 路径上，把破坏性步骤推迟到授权之后 —— 在本次读取之前先等待自然结束（以 drainTimeoutMs 为界的 waitForActiveTurnsToSettle()），只有当两次读取都为空时才运行 generation-controller abort 和 cancelPendingPrompt()（紧接在 recorder?.finalize() 之前）；或者在拒绝路径上恢复 cron scheduler 并重建被清空的队列状态。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| capability && | ||
| !owner.isQuarantined && | ||
| !owner.restoreSettlementOverdue && |
There was a problem hiding this comment.
[Suggestion] The new incomplete-reporting guard runs ahead of the shared automatic-cleanup funnel's deferred spawn-owner kill branch in maybeCloseIdleSession (bridge.ts:2284), so an explicitly requested kill is silently never executed for sessions owned by a negotiated-but-incomplete child — even when no shell is running.
Failure scenario: new daemon + old v1 child (responds categories: ['agent','notification']). A spawn owner's transport drops while another client is attached → killSession(..., {requireZeroAttaches: true}) sets the spawnOwnerWantedKill tombstone → when the last attached client later detaches, entryIsAutoCloseCandidate returns false before reaching the tombstone branch; maybeCloseIdleSession is the tombstone's only resolution site and the reaper funnels through the same guard → nothing ever resolves it (the child is healthy, so it is never condemned), and the session and channel are retained indefinitely despite an explicit kill request. The kill path consults no child predicate — kill is a force operation everywhere else — so the guard protects nothing here while contradicting the design doc's "kill … keep their force semantics".
Suggested fix: handle entry.spawnOwnerWantedKill && entry.attachCount === 0 before this new guard (killSession is force and never trusts the child's answer), or explicitly document that deferred owner-requested kills are also suspended for incomplete children.
中文说明
新的“上报不完整”保护位于共享自动清理漏斗中延迟的 spawn-owner kill 分支（bridge.ts:2284）之前，因此对于协商过但类别不完整的 child 拥有的 session，显式请求的 kill 会被静默地永远不执行 —— 即使没有任何 shell 在运行。
失败场景：新 daemon + 旧 v1 child（响应 categories: ['agent','notification']）。spawn owner 的 transport 在另一个 client 仍 attached 时断开 → killSession(..., {requireZeroAttaches: true}) 设置 spawnOwnerWantedKill tombstone → 当最后一个 attached client 随后 detach 时，entryIsAutoCloseCandidate 会在到达 tombstone 分支之前返回 false；maybeCloseIdleSession 是该 tombstone 唯一的解决点，而 reaper 也经过同一个保护 → 该 tombstone 永远不会被解决（child 健康，因此不会被 condemn），session 和 channel 尽管有显式 kill 请求仍被无限期保留。kill 路径不询问 child 的任何谓词 —— kill 在其他所有地方都是强制操作 —— 所以该保护在这里没有保护任何东西，却与设计文档中“kill……保持强制语义”相矛盾。
建议修复：在这个新保护之前处理 entry.spawnOwnerWantedKill && entry.attachCount === 0（killSession 是强制操作，从不信任 child 的回答），或者明确记录：对类别不完整的 child，延迟的 owner 请求 kill 同样被暂停。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ACTIVE_WORK_HOLD_CATEGORIES.some( | ||
| (category) => !capability.categories.includes(category), | ||
| ) |
There was a problem hiding this comment.
[Suggestion] The "negotiated capability covers every required category" predicate is now hand-inlined twice in this file: the pre-existing copy inside activeWorkCoverage (health grading, bridge.ts:6749-6753) and the identical copy this diff adds to the cleanup-candidate guard.
Concrete cost: the two call sites answer the same question for two consumers (/health?deep=1 grading and the automatic reaper), and the design docs anticipate category-set growth — when the predicate semantics change (a newly added category should degrade the health grade but is not yet required for cleanup, or a required/advisory distinction is introduced), the edit must land in both expressions; missing one makes the reaper and the health surface disagree about the same session (health reports partial while cleanup still authorizes teardown, or vice versa), silently, because both sites compile.
Suggested fix: extract one predicate — e.g. reportsAllActiveWorkCategories(capability) in bridgeTypes.ts, which both call sites already import — and call it from both the cleanup guard and activeWorkCoverage.
中文说明
“协商能力覆盖全部必需类别”这一谓词现在在本文件中被手工内联了两次：activeWorkCoverage（健康分级，bridge.ts:6749-6753）中的既有副本，以及本 diff 为清理候选保护新增的相同副本。
具体代价：两个调用点为两个消费者（/health?deep=1 分级与自动 reaper）回答同一个问题，而设计文档已预期类别集合会增长 —— 当谓词语义变化时（新增类别应降低健康分级但尚不要求用于清理，或引入必需/建议的区分），修改必须同时落在两个表达式上；漏掉一个会让 reaper 与健康面板对同一个 session 给出不一致的结论（健康报 partial 而清理仍授权 teardown，或反之），而且因为两处都能编译，这种分歧是静默的。
建议修复：抽取一个谓词 —— 例如放在两个调用点都已 import 的 bridgeTypes.ts 中的 reportsAllActiveWorkCategories(capability) —— 并在清理保护和 activeWorkCoverage 中调用它。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| ? ACTIVE_WORK_HOLD_CATEGORIES.filter((category) => | ||
| requestedActiveWorkCategories.includes(category), | ||
| ) |
There was a problem hiding this comment.
[Suggestion] This child-side category-intersection parse duplicates the daemon-side parse at bridge.ts:3581-3585 — the same ACTIVE_WORK_HOLD_CATEGORIES.filter expression across the two endpoints of this one handshake, with only the non-array fallback differing (legacy baseline vs [], deliberately, per wire direction). The interval half of this very handshake already sets the precedent the categories half skips: clampActiveWorkIntervalMs is a shared helper in bridgeTypes.ts used by both sides.
Concrete cost: this expression defines what counts as a negotiated category on the active-work v1 wire; if the intersection semantics must change (stricter element validation, or echoing unknown categories forward for a future probe), the edit has to be made independently in two packages that are the two endpoints of the same handshake — missing one side makes a new child and new daemon compute different negotiated sets for the same exchange, which is exactly the version-skew drift category negotiation exists to prevent.
Suggested fix: add a shared helper in packages/acp-bridge/src/bridgeTypes.ts (both files already import from it), e.g. intersectActiveWorkCategories(raw: unknown): ActiveWorkHoldCategory[] | undefined returning undefined for a non-array, and let each side apply its own direction-specific fallback ([] daemon-side, ACTIVE_WORK_LEGACY_HOLD_CATEGORIES child-side).
中文说明
child 侧新增的类别交集解析与 daemon 侧 bridge.ts:3581-3585 处的解析重复 —— 同一个握手的两个端点使用相同的 ACTIVE_WORK_HOLD_CATEGORIES.filter 表达式，只有非数组回退不同（按 wire 方向有意区分：legacy 基线 vs []）。同一握手中 interval 的一半已经树立了被 categories 一半跳过的先例：clampActiveWorkIntervalMs 是 bridgeTypes.ts 中双方共用的 helper。
具体代价：该表达式定义了 active-work v1 wire 上什么算作已协商类别；如果交集语义必须改变（更严格的元素校验，或为未来探测回显未知类别），就必须在同一握手两个端点所在的两个包中独立修改 —— 漏掉一端会让新 child 与新 daemon 对同一次交互算出不同的协商集合，这正是类别协商机制要防止的版本漂移。
建议修复：在 packages/acp-bridge/src/bridgeTypes.ts（两个文件都已 import 它）中新增共享 helper，例如 intersectActiveWorkCategories(raw: unknown): ActiveWorkHoldCategory[] | undefined（非数组返回 undefined），由两端各自应用方向特定的回退（daemon 侧 []，child 侧 ACTIVE_WORK_LEGACY_HOLD_CATEGORIES）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| const capability = owner?.activeWork; | ||
| if ( | ||
| capability && | ||
| !owner.isQuarantined && |
There was a problem hiding this comment.
[Suggestion] The !owner.isQuarantined escape hatch in this new guard has no test. Probed empirically: deleting the clause survives the full bridge suite (577/577 tests still pass). The clause is not dead — channel.isQuarantined is set at bridge.ts:5948 after a failed late-restore cleanup, and quarantine can coincide with an incomplete capability (new daemon + old child).
Failure scenario: a channel quarantined after a failed late-restore cleanup whose child negotiated only agent/notification, holding an idle detached session → ordinary auto cleanup is expected to stay enabled for quarantined channels (that is what the carve-out encodes), but with the clause deleted such sessions become permanently unreapable by ordinary cleanup, and no existing test would fail.
Suggested fix: add a bridge test combining the quarantine setup from 'quarantines only fresh work when late restore cleanup fails' with a legacy-categories activeWorkInitializeResponse, then detach and assert a conditional close is attempted / the session is reaped.
中文说明
这个新保护中的 !owner.isQuarantined 豁免分支没有测试。经实证探测：删除该子句后整个 bridge 测试套件仍然通过（577/577）。该子句并非死代码 —— channel.isQuarantined 会在 late-restore 清理失败后于 bridge.ts:5948 被设置，而 quarantine 状态可能与不完整能力并存（新 daemon + 旧 child）。
失败场景：一个在 late-restore 清理失败后被 quarantine 的 channel，其 child 只协商了 agent/notification，持有一个空闲 detached session → 普通自动清理对 quarantined channel 应保持启用（这正是该豁免分支编码的语义），但若该子句被删除，这类 session 将永远无法被普通清理回收，而现有任何测试都不会失败。
建议修复：新增一个 bridge 测试，把 'quarantines only fresh work when late restore cleanup fails' 中的 quarantine 布置与 legacy 类别的 activeWorkInitializeResponse 组合，然后 detach 并断言会尝试 conditional close / session 被回收。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| sessionId: 'legacy-active-work-session', | ||
| closed: false, | ||
| holds: [{ category: 'shell', id: 'background-shells' }], |
There was a problem hiding this comment.
[Suggestion] No assertion in this test discriminates the early under-gate hold rejection from the post-drain rejection. Probed empirically: deleting the first collectActiveWorkHolds() read (acpAgent.ts:4354-4359) survives the entire acpAgent suite (395/395); adding the assertions below fails the mutant and passes on the unmodified code.
Failure scenario: the drain-phase mocks (waitForActiveTurnsToSettle, cancelPendingPrompt) are no-ops returning success, so with the early read deleted the second read rejects with the identical {closed: false, holds} payload and this test still passes. Under that mutant in production, a conditional close on a session with already-known holds first runs the destructive drain phase — aborting generation controllers, calling session.cancelPendingPrompt(), and waiting up to SESSION_DRAIN_TIMEOUT_MS (30s) — instead of rejecting immediately, past the daemon's ACTIVE_WORK_CLOSE_TIMEOUT_MS (10s) budget where an instant rejection would have fit.
Suggested fix:
expect(lastSessionMock?.waitForActiveTurnsToSettle).not.toHaveBeenCalled();
expect(lastSessionMock?.cancelPendingPrompt).not.toHaveBeenCalled();中文说明
本测试中没有任何断言能区分“close gate 下的提前拒绝”与“drain 之后的拒绝”。经实证探测：删除第一次 collectActiveWorkHolds() 读取（acpAgent.ts:4354-4359）后整个 acpAgent 套件仍然通过（395/395）；加入下方断言后该变异体会失败，而在未修改代码上通过。
失败场景：drain 阶段的 mock（waitForActiveTurnsToSettle、cancelPendingPrompt）是返回成功的空操作，因此删除提前读取后，第二次读取会以相同的 {closed: false, holds} 载荷拒绝，本测试依然通过。在生产环境下若存在该变异体：对已有已知 hold 的 session 执行 conditional close 时，会先运行破坏性的 drain 阶段 —— 中止 generation controllers、调用 session.cancelPendingPrompt()、最多等待 SESSION_DRAIN_TIMEOUT_MS（30 秒）—— 而不是立即拒绝；这超出了 daemon 的 ACTIVE_WORK_CLOSE_TIMEOUT_MS（10 秒）预算，而立即拒绝本可以在预算内完成。
建议修复：
expect(lastSessionMock?.waitForActiveTurnsToSettle).not.toHaveBeenCalled();
expect(lastSessionMock?.cancelPendingPrompt).not.toHaveBeenCalled();— qwen3.8-max via Qwen Code /review (v0.21.11)
Independent verification report (local real-stack A/B)Verified Result: every claim in the PR description reproduced. No regressions found. LGTM from my side. 1. Deep health during the shell lifecycle (before/after)Base: with the prompt settled and the shell task running (real PID alive), deep health reads
2. The harm, end to end (idle reaper, TTL 2s, client detaches with shell running)Base: session reaped 2s after detach, the running
3. Cross-version compatibility (real old/new binaries via
|
|
@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: 38 passed · 0 failed · 38 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：38 通过 · 0 失败 · 38 总计 Verification reportPR 9042 Deep Verification — feat(daemon): Track background shells in activeWorkVerdict: 中文摘要结论： A/B 结论（见下表及
Findings：无阻塞项。两条非阻塞观察（见正文 Findings）：① base 侧 未覆盖：真实模型驱动的端到端 deep-health（沙箱无模型凭据，链路逐段实测替代）；Windows/macOS 差异；Monitors/workflows/cron/PID 探测（PR 声明范围外）；仓库全量测试。 Central claim and A/B evidenceCentral claim: while a Session-managed background shell is running — or its terminal notification is queued or driving the parent continuation — deep health reports The chain was proven segment by segment, A/B on both sides: Child side (Session → reporter): the load-bearing flipThe PR's own new tests were run against both trees (identical test files; only production source differs — test copies in the base tree; vitest aliases resolve every
Witness: Daemon side (bridge): real bridge over in-memory NDJSON channels
Head arm 10/10, base arm 9/9 (arm-aware expectations encoded in the harness). Witness: Two facts from C2 sharpen the defect localization: (a) the base daemon was already generic enough to hold arbitrary categories — the pre-PR defect is entirely child-side (no shell hold was ever emitted); (b) base would not even have tolerated a hypothetical shell report — the whole-snapshot rejection means the session would read "unknown", go to Reviewer Test Plan, per step
CorrectionsNone needed for earlier review rounds. One correction to the reading of my own first base-arm run, applied before the final numbers: the base C2 FindingsNo blocking findings.
Not covered
MethodologyEnvironment: the CI merge-ref checkout ( Evidence 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 bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved as requested.
Already have 2 approves，3ks.
|
Released in v0.21.12. |
4 participants
What this PR does
This PR extends the existing
activeWorkretention fact to Session-managed background shells. A Session reports one boundedshellhold while any registered shell is running, while a shell terminal notification is queued, or while that notification is driving its parent continuation. The full local collector remains independent of wire negotiation, while the active-work reporter filters snapshots to the categories negotiated with its daemon peer.The active-work v1 handshake now negotiates
agent,notification, andshellcategories explicitly. A new daemon paired with an older v1 child reports partial coverage and disables ordinary automatic cleanup for that Session; a new child paired with an older daemon sends only the legacy categories on the wire but still rejects conditional close locally while shell work exists. Explicit close, kill, shutdown, channel exit, and condemned restore cleanup keep their force semantics.Why it's needed
A Prompt can start a long-running background shell and settle immediately. Before this change, deep health could then report
activePrompts: 0,activeWork: false, and full reporting while the shell task was still running. A restart controller could classify the daemon as idle, close the Session, and prevent the shell terminal notification from reaching the parent continuation.Using one aggregate shell hold keeps the retention protocol bounded and leaves detailed shell state on the existing task-status surface. Rechecking unfiltered holds after active turns drain also closes the conditional-close window in which an already-running cron or automatic turn can register a shell while teardown is waiting.
Reviewer Test Plan
How to verify
Start a long-running background shell from a Prompt and let the Prompt settle. Confirm that deep health reports
activePrompts: 0,activeWork: true, andactiveWorkReporting: "full"while the task surface reports the shell as running. Detach the client and confirm that ordinary cleanup preserves the Session. Let the shell finish while blocking its parent continuation and confirm thatactiveWorkstays true through the running-to-queued-to-continuation handoff, then becomes false only after the continuation settles.Also verify compatibility behavior: a negotiated child that omits
shellproduces partial reporting and is excluded from ordinary automatic cleanup; a completely unsupported child keeps legacy cleanup; explicit close and kill remain forceful. Unit coverage passed for the shell registry (61 tests), active-work reporter and ACP initialization (405 tests), Session lifecycle and notification handoff (593 tests), and ACP bridge retention and compatibility (577 tests).Evidence (Before & After)
Before: with a Session-managed
sleep 120task still reported as running, deep health returnedactivePrompts: 0,activeWork: false, andactiveWorkReporting: "full".After: the same state returns
activePrompts: 0,activeWork: true, andactiveWorkReporting: "full";activeWorkremains true until the shell terminal continuation settles.Tested on
Environment (optional)
macOS, Node.js 24.12.0, package-level Vitest and TypeScript checks without sandboxing.
Risk & Scope
activeWork: true; negotiated older children are retained rather than automatically cleaned up when their category coverage is incomplete.ink+7.0.3patch, which leaves unrelated CLI text-selection types unavailable; affected core and ACP bridge typechecks, lint, formatting, and all directed behavior tests passed.Linked Issues
Refs #8586
中文说明
本 PR 的变更
本 PR 将现有
activeWork保留事实扩展到 Session 管理的后台 shell。当任一已登记 shell 正在运行、shell 终态通知仍在队列中，或该通知正在驱动父 Agent continuation 时，Session 上报一个有界的shell聚合 hold。Session 本地的完整 collector 不受 wire 协商影响，只有 active-work reporter 在生成快照时按与 daemon 对端协商的类别进行过滤。active-work v1 握手现在显式协商
agent、notification和shell类别。新 daemon 与旧 v1 child 配对时会报告 partial coverage，并禁止该 Session 的普通自动清理；新 child 与旧 daemon 配对时，wire 上只发送旧类别，但本地存在 shell 工作时仍会拒绝 conditional close。显式 close、kill、shutdown、channel exit 和 condemned restore cleanup 继续保持强制语义。为什么需要
Prompt 可以启动长时间运行的后台 shell 后立即结束。变更前，即使 shell 任务仍在运行，deep health 也可能返回
activePrompts: 0、activeWork: false和 full reporting。重启控制器可能因此把 daemon 误判为空闲、关闭 Session，并阻止 shell 终态通知到达父 Agent continuation。单一 shell 聚合 hold 能让保留协议保持有界，详细 shell 状态仍由现有任务状态接口提供。在 active turn 排空后再次检查未过滤 hold，也关闭了 conditional-close 窗口：已经运行的 cron 或自动 turn 可能在 teardown 等待期间登记新的 shell。
Reviewer 测试计划
验证方式
从 Prompt 启动一个长时间运行的后台 shell，并让 Prompt 结束。确认任务接口仍显示 shell 运行时，deep health 返回
activePrompts: 0、activeWork: true和activeWorkReporting: "full"。detach 客户端并确认普通清理仍保留 Session。让 shell 结束但阻塞父 continuation，确认activeWork在 running、queued 和 continuation 的整个交接过程中始终为 true，并且只在 continuation 完成后变为 false。同时验证兼容行为：协商后缺少
shell的 child 产生 partial reporting，并且不参与普通自动清理；完全不支持 active-work 的 child 继续使用旧清理行为；显式 close 和 kill 仍为强制操作。shell registry 61 项、active-work reporter 与 ACP 初始化 405 项、Session 生命周期与通知交接 593 项，以及 ACP bridge 保留与兼容 577 项单元测试均已通过。证据（Before & After）
Before：Session 管理的
sleep 120任务仍显示 running 时，deep health 返回activePrompts: 0、activeWork: false和activeWorkReporting: "full"。After：同一状态返回
activePrompts: 0、activeWork: true和activeWorkReporting: "full"；直到 shell 终态 continuation 完成，activeWork都保持 true。测试平台
环境（可选）
macOS、Node.js 24.12.0，未启用 sandbox，运行 package 级 Vitest 和 TypeScript 检查。
风险与范围
activeWork: true；已协商但类别覆盖不完整的旧 child 会被保留，而不会被普通自动清理。ink+7.0.3补丁所阻断，导致与本 PR 无关的 CLI 文本选择类型缺失；受影响的 core 与 ACP bridge typecheck、lint、格式检查及所有定向行为测试均通过。关联 Issue
Refs #8586