Repository navigation
fix(mods): exclude native waits from root execution budget - #14986
Conversation
|
P02b implementation verification report (local evidence; hosted CI and formal acceptance are separate). Snapshot
Before The test engineer reproduced the original long-wait defect before implementation. Installed CLI 0.25.3: Mods enabled cancelled at 60059.7ms with only an oracle start; disabled control completed at 66001.9ms. The frozen source base separately reproduced Ink approval expiry around 61 seconds. Global CLI and source-base evidence have distinct version/hash identities. The initial candidate exposed a pre-existing Ink cancellation-history defect: exact PID stopped and JSONL recorded cancellation, but NEXT received a missing-result placeholder. The frozen base reproduced that same defect. Two new ordinary-cancellation regressions failed before the minimal fix (zero history writes) and passed afterward, covering all-cancelled and same-owner mixed batches. The original red-test record is a labeled tool-output excerpt. Final artifact matrix
All 41 unique cases passed on the first execution. At the 65-second native sample, ENTER was visible and completed/SETTLED/paired results were absent. The oracle start/completion share the same PID; its monotonic duration proves foreground execution crossed the old limit. No case needed a forced oracle cleanup. Ink took 410.16s and OpenTUI took 337.53s. Every per-case artifact hash matched before/after. The complete matrix includes rewriting, deny/synthetic results, repeated physical attempts, catch replay, withholding, native permissions, long approval, cancelled approval with late Enter, long foreground execution, explicit 5000ms native timeout, execution cancellation, batch, disabled and command-only behavior. Ink includes nine headless cases; strict OpenTUI does not permit renderer fallback. Each interactive case checks NEXT and natural exit, final recording facts and result pairing. Headless cancellation emits one error tool result, exits 130 and makes no continuation model request. The separate test engineer independently checked final Ink Esc cancellation: oracle PID stopped after 50.8ms; only Build and focused regressions
The core and seven CLI boundary runs used the first committed candidate; the follow-up changed only ordinary Ink history pairing, its two regressions and the direct-worker bundle fixture. The relevant unchanged component hashes match the final head. The final source was fully rebuilt/typechecked, the whole 303-case hook file and bundle contract passed, and the runtime matrix used the final artifact hashes above. Counts are separate tests, not a claim to have run the repository-wide suite. Deterministic tests cover the 5s charged + 90s excluded + 7s charged ledger, repeated/nested loans, exhausted budgets, wrong roots/operations/revisions, queued work, charged permission checks, stale idle after check cancellation, and a worker termination at 250ms while native cleanup is intentionally held. Real QuickJS worker tests observe idle for awaited native work and charging for hanging checks, log observers and continued Promise jobs, plus cancellation followed by a healthy root. These worker cases verify activity transitions rather than directly accelerating a complete 60-second worker expiry. Existing own/catch, execution-fact and lifecycle tests remain part of the focused runs. Commands (package tests run from the named package directory): npm run build
npm run typecheck
npm run bundle
# packages/core
npx vitest run src/mods/budget.test.ts src/mods/runtime-budget.test.ts src/mods/worker-budget.test.ts src/mods/runtime.test.ts src/mods/runtime-worker.test.ts src/mods/tool-runtime.test.ts src/mods/tool-execution.test.ts src/mods/dispatch.test.ts
# packages/cli
npx vitest run src/services/mod-session.test.ts src/services/interactive-mod-session.test.ts src/services/interactive-mod-tool-scheduler.test.ts src/services/interactive-mod-tool-scheduler.native.test.ts src/ui/hooks/useResumeCommand.test.ts src/ui/hooks/useBranchCommand.test.ts src/ui/opentui/session-switch.test.ts
npx vitest run src/ui/hooks/use-llm-stream.test.tsx --coverage.enabled=false
# repository root
npx vitest run --config scripts/tests/vitest.config.ts scripts/tests/mod-worker-bundle.test.js
# integration-tests
CI=true QWEN_SANDBOX=false QWEN_E2E_RENDERER=ink npx vitest run interactive/mods-tool-middleware.test.ts
CI=true QWEN_SANDBOX=false QWEN_E2E_RENDERER=opentui npx vitest run interactive/mods-tool-middleware.test.tsEvidence and boundaries Raw local evidence is retained under ignored Windows/Linux runtime and live model services were not tested locally. The 250ms requirement bounds worker termination, not the duration of native process/task cleanup. P02c, hosted Mods and broader post-attachment cancellation changes are outside this slice. No review approval, merge or hosted CI success is claimed by this report. |
| // this branch owes its own batch the same pairing, whether or not | ||
| // every tool in it was cancelled. | ||
| if (toolGoalBinding && llmClient) { | ||
| if (llmClient) { |
There was a problem hiding this comment.
[P2] 取消结果补配对必须保持原会话归属
独立验收基于 efc5928d8bced881b6804113ae7e52bd1908bf20。这里去掉 toolGoalBinding 条件后，普通工具取消会直接向当前 llmClient 补写结果，但前面的 await finalizeToolResponses(...) 可以跨越 /clear。这是本轮唯一确认的阻塞问题，违反 P02b 的旧会话结果不得进入新会话的约束。
已复现的顺序：
- Mod 返回 220000 个 ASCII 字符的合成结果，超过默认 200000 字符 batch budget、低于 Mod 262144 字节 payload 上限；尚无
persistedOutputFiles，真实 finalizer 进入异步落盘。 - 落盘等待期间首次按 Esc。Mod adapter 已在调用 UI 完成回调前清空工具显示，取消后 Hook 进入 Idle，随后
/clear被正常接纳。 /clear切换 session 并调用同一客户端的resetChat()。ModSession.syncSession()只等待内部runtime.runTool所在的 queue，不等待外层 UI finalizer，因此不会阻止这一步。- 释放磁盘 I/O 后，旧 continuation 的 signal 仍为 aborted，进入这里调用
addHistory()。它动态使用getChat()，而resetChat()已替换内部 chat，于是旧 callId 的functionResponse被追加到新会话。入口处的历史去重快照不能保护这个 await 之后的窗口。
定向 A/B 使用实际 useLlmStream、clearCommand.action、finalizeToolResponses 和落盘函数，仅在目标 fs.mkdir 上加 gate 控制时序，并验证真实文件写入了 220000 字符。当前 head 的“新 chat 应为空”断言失败，实际多出 old-session-tool；同一探针仅恢复原 toolGoalBinding && llmClient 条件后通过。test-engineer 已独立复跑两组并得到相同结果。scheduler、config 和客户端 chat 替换使用受控替身，其语义已与生产代码核对；这是受控回归探针，不是完整终端自然触发该 race 的 E2E。
请在异步收尾后重新确认原工具 continuation 的 session/chat 归属与当前配对状态，再执行补配对：同一会话取消仍需补齐，已经切换会话则不能向新 chat 写入旧结果。建议添加上述“finalizer 挂起 → 首次取消 → clear → finalizer 完成”的回归，并保留普通取消后的下一轮继续测试。恢复旧条件仅用于定位回归来源，不建议以直接回退来丢掉本 PR 的普通取消配对修复。
其余独立验证通过：build、bundle、typecheck，core 130/130、CLI 462/462、bundle contract 1/1；真实 Ink 16 + strict OpenTUI 16 + headless 9，共 41/41 首轮通过、0 retry，覆盖 ≥65s 审批、≥66s native 执行、原生 timeout 和取消。额外真实 worker 负例在 native pending 且 guest 持续运行 Promise jobs 时于约 60.002s 耗尽根预算。因此预算主体的验证通过，但上述会话归属回归修复前，本轮整体验收不通过。
There was a problem hiding this comment.
已核实，这条 P2 有效，并在修复前的 efc5928d8b 独立复现：真实 finalizer 写入 220000 字节后，新 chat 错误包含 old-session-tool。修复已推送到 63c4956b2014994050e29883ec04f061f0fd8d50。
- 完成回调入口捕获原 session ID 和 chat 实例；内存刷新结束后、真实 finalizer 结束后分别检查，旧归属直接退出，防止污染新会话的历史、结果记录、完成统计或 submitted 标记。
- 旧批次已有 Goal 绑定时，提前退出只通过现有、按对象/controller 身份保护的本地 helper 释放旧引用，不调用新会话的 Goal runtime。两例先红后绿的回归还验证了下一普通轮 Esc 不会被误判为 Goal 取消。
- 同一会话的普通取消仍补配对，并在 append 前重新读取已配对 callId，按 finalized entry 整体过滤，保留其伴随 parts、避免重复补写。
本轮验证：完整 build/typecheck/bundle；hook 文件 310/310（7 个新增边界场景及原普通取消/混合用例）；clear/scheduler 边界 29/29；改动文件 lint/format 和两轮完整 diff 自查通过。最终提交的 Ink terminal 16/16 首次通过、零重试；长审批 65.063s 后批准成功，前台原生执行同 PID 持续 66.029s，65s 采样时尚未结算。原生 5s timeout、执行中取消、一次结果/记录、NEXT 和自然退出均通过。独立探针对原 efc hook 为 1/1 红、最终 source 为 1/1 绿（均真实落盘 220000 字节；修复后新 chat 及新记录无旧结果）；最终 bundle 的独立真实 Ink 取消验证也通过，产物前后 hash 一致。
范围取舍也明确记录：没有回退到旧的 Goal-only 条件，A/B 里恢复它只是定位回归，不能作为修复；没有新增调度期身份协议或扩展成所有原生 fallback/附件续体的重构。本次处理的是已进入完成回调后跨异步等待发生的归属变化。原问题的精确时序采用受控 Hook/真实 finalizer/真实落盘探针验证，scheduler/config/chat 为受控替身，不将其称为自然终端 race E2E。
已分页核对公开 review、线程和普通评论，本轮仅这一条有效问题，没有其他需要拒绝或延期的 review 条目。原提交的 Ubuntu 单测、Lint、no-AK、TUI 和 Managed Agent gate 已通过；两项 queued/pending 前检没有执行步骤或断言日志，没有据此修改代码或盲目重跑。新提交的 CI 状态另按实际回读报告。
|
PR #14986 review-fix verification update. This supplements the original Disposition: the sole public P2 review finding was confirmed and repaired. Independent exact-head reproduction showed real persistence of 220000 bytes followed by one old result in the new chat after Esc → Final snapshot:
The new early-exit Goal cleanup was verified red/green: before releasing the old binding, a new ordinary turn's Esc incorrectly reported Scope decisions: the Goal-only A/B condition was not restored, because that would lose ordinary cancellation pairing. No public API/configuration, budget protocol, scheduler-wide identity mechanism or unrelated cancellation/attachment rewrite was added. There are no other public review items to reject or defer in the paginated snapshot. The precise race is controlled test-script evidence using real Hook/clear/finalizer/persistence and conforming substitutes for scheduler/config/client; it is not a naturally timed terminal race. Windows/Linux runtime were not locally tested. This repair does not claim stale-entry coverage for every unchanged native fallback. Original-head hosted Ubuntu Node 22 tests, lint/static, no-AK integration, Web Shell smoke, TUI parity/no-flicker and Managed Agent gate passed. macOS/Windows test legs were skipped. The two prechecks remain queued/pending without steps/assertion logs; that is not diagnosed as a code failure. New-head hosted snapshot (2026-10-21 06:43 UTC): Qwen Code CI, Managed Agent gate, and TUI parity are in progress; the new automatic review is pending. No new-head failure is present in that snapshot. The PR remains draft, MERGEABLE + BLOCKED, with REVIEW_REQUIRED. Hosted CI and formal review remain separate from local verification. Evidence is retained in ignored |
|
Qwen Code review never started. The job ended before a runner picked it up, so no review ran for this head. This can happen when a queued run is cancelled or expires; this step cannot determine the cause. See workflow logs. Check the latest |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Summary
P02b changes the 60-second Mod tool-root limit to cumulative charged time, excluding native execution and approval waits when the host owns the active attempt and no guest work is running. The required cancellation fixture also corrects an Ink result-pairing bug for cancelled turns, and adds /clear crossing protection for async result persistence.
Key changes
- Budget accounting (
dispatch.ts):ModBudgetgains ref-countedpause()/resume()and alendinggetter, replacing the old monolithiclend(). - Worker activity tracking (
mod-worker.ts): NewupdateActivity()computes whether the worker is idle enough to lend budget — conservatively requiring no guest execution, checks, notifications, jobs, or non-lending calls. Activity messages carry monotonic phase/revision to reject stale state. - Host root tracking (
runtime.ts): Each dispatch creates a root with its ownModBudget. The host pauses/resumes via activity protocol, and the 250ms cancellation watchdog stays independent. - Result pairing (
use-llm-stream.ts): Cancelled ordinary turns now pair finalized real responses (filtering already-paired callIds). Async completion revalidatesisCurrentSession()to avoid crossing/clear. - Tests: New
budget.test.ts,runtime-budget.test.ts,worker-budget.test.ts, plus expanded integration tests covering long-approval, long-native, approval-cancel, cancel, and native-timeout modes. - Docs:
mods.mdupdated with accurate cumulative-budget semantics.
Scope & linkage
- P2: No linked issue (references #13749 but does not close it)
Verdict
APPROVE. The budget accounting is conservatively designed — lending only when the worker confirms idle via monotonic phase/revision messages. The activity protocol correctly rejects stale messages by root/phase/revision. The cancellation result pairing fix is minimal and well-tested. No P0 or P1 findings. CI is green on current head 064c31f.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Final gate review at head 064c31f; head SHA and the check rollup re-read immediately before submitting.
Historical blocking issue: resolved and verified in the code at this head. The one P2 on record — a cancelled tool batch writing its pairing functionResponse into whatever chat getChat() returned, so an old session result could land in the new chat after Esc then /clear — was fixed in 63c4956b2, an ancestor of this head. handleCompletedTools captures the session id and the chat instance at entry and re-validates both at the two boundaries that can span a session switch (after the memory refresh, and after the real finalizer, ahead of any addHistory), returning early and releasing the goal binding through the pre-existing releaseGoalTurn helper on stale ownership. The cancelled-continuation branch still pairs its own batch, but now filters against getHistoryFunctionResponseIds so an already-paired callId is not written twice.
Required CI green at this head: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, TUI parity snapshots (ink vs opentui), OpenTUI no-flicker gate, Managed Agent gate and Select the Managed Agent tier all success; the rest skipped. The only check still running is review-pr, the automated review workflow itself: it is triggered by pull_request_target and pull_request_review, so this very submission re-queues it and it can never be complete beforehand. The main ruleset carries only deletion and non_fast_forward — no required_status_checks.
No new Critical found. The mechanism is guarded on both sides of the wire:
- ModBudget.lend splitting into pause/resume keeps the identical 0-to-1 transition accounting, so existing lenders are unaffected.
- The root budget stops being charged only when both sides agree a trusted native wait is in flight. The worker reports activity only when genuinely idle — no executing slice, no outstanding checks, no guest notification other than the next that owns the wait, no pending QuickJS job, and no live call that is not already lending. The host pauses only when root.idle equals root.native, no tool check is outstanding, and the budget has not expired.
- Stale traffic is rejected in both directions: the host drops any reply whose root differs and posts only for the live root; the worker drops native, host-result and tool-check messages for a stale root or a lower phase, and ignores a root id at or below its current one.
- A monotonic phase — bumped when a native attempt starts, when it settles, and around each check — plus a strictly increasing revision prevent a late idle report from pausing the budget after the wait it describes has ended.
- The checks counter is decremented only when the request root matches the live root, so a check outliving its root cannot corrupt its successor, and lastCheck dedups a replayed check id.
- Only tool roots exclude waits; load, command and end roots keep a continuous limit, exactly as docs/users/extension/mods.md now states.
- Cancellation is untouched by the exclusion: onAbort arms its 250ms stop timer regardless of lending, refresh no-ops once aborted or superseded, and the worker acknowledgement watchdog is documented as independent of the budget. The finally block clears the root, both timers and the abort listener.
- The protocol change is internal: the worker is the repo-built bundle resolved through resolveBundleDir and pinned by scripts/tests/mod-worker-bundle.test.js, updated here to stamp and validate root and phase, so host and worker always ship together — no third-party wire compatibility break.
- Coverage was added where the risk is: budget.test.ts, runtime-budget.test.ts and worker-budget.test.ts plus the hook and integration suites.
One deliberate trade, documented rather than blocked: while a tool root is lending no wall-clock timer is armed, so the 60 second ceiling no longer bounds a hung native attempt. The escapes are the native tool timeout policy, the permission decision, and Esc followed by termination at 250ms; mods.md states this explicitly along with the fact that starting another attempt never resets the remaining budget.
5 participants
What this PR does
P02b changes the 60-second Mod tool-root limit to cumulative charged time on both the host and worker. Actual native execution and approval waits are excluded only while the trusted bridge confirms that guest work is idle. Guest checks, continuations, jobs and log observers remain charged; starting another attempt never refreshes the allowance. Root/phase identities reject stale activity, and cancellation retains its independent 250ms worker watchdog.
The required cancellation fixture also exposed an existing Ink result-pairing bug: Esc stopped the native process and recorded cancellation, but the next turn received a missing-result placeholder. A minimal correction now pairs the finalized real responses for ordinary cancelled turns, including mixed success/cancelled batches. Review also identified a completion crossing
/clearduring asynchronous result persistence. Completion now revalidates the original session and chat before bookkeeping/recording, releases stale local Goal state, and skips already-paired responses. The user documentation and existing no-AK E2E matrix describe and exercise the new behavior.Why it's needed
Both outer clocks previously counted native waiting as Mod execution. A legitimate long-running tool or an unanswered approval could therefore be cancelled around 60 seconds even when the plugin itself was idle. Pausing only one clock or increasing the limit would leave the underlying accounting and cancellation boundaries incorrect.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: the installed 0.25.3 CLI cancelled the enabled-Mod native oracle at 60059.7ms; its disabled-Mod control completed at 66001.9ms. A separate frozen-base Ink build lost the approval around 61 seconds and reproduced the cancelled-result placeholder. These are distinct, explicitly recorded baselines.
After: The original budget candidate
efc5928d8bpassed all 41 artifact cases on the first attempt (16 Ink + 9 headless; 16 strict OpenTUI), with zero retries. Approval remained actionable after 65.098s / 65.077s. Foreground native execution completed at 66.011s / 66.007s / 66.028s for Ink / OpenTUI / headless, with no premature result at the 65-second sample. Native timeouts, cancellation, result pairing, NEXT and exit checks passed, and artifact hashes were unchanged.Repair head
63c4956b2014994050e29883ec04f061f0fd8d50: Final-head Ink terminal regression passed 16/16 on the first attempt with zero retries: approval remained actionable after 65.063s, foreground native execution lasted 66.029s in the same process, and the 65s sample had no settled/completed/paired result. Native timeout, cancellation, result/record pairing, NEXT and natural exit passed. The independent conforming probe was red on the original hook and green on the final source, with actual 220000-byte persistence in both and no old result in the replacement chat/recorder after repair. Independent final-bundle Ink cancellation also passed; all head/artifact hashes were unchanged.Validation on the repair head: complete build, typecheck and bundle; 310 stream-hook cases, 29 clear/scheduler boundary cases, and changed-file ESLint/Prettier. The original budget head also passed 130 core cases, 159 CLI boundary cases and one relocated-worker bundle contract case; those unchanged-component results retain their original head identity. The real-worker unit cases verify activity transitions; cumulative expiry uses deterministic ledger/protocol tests, with real long waits checked separately by E2E. A separate test engineer reproduced the original issue and independently verified the long-wait candidate and final Ink cancellation. Final hashes and commands are in the separate E2E report comment. Local validation and source review do not imply hosted CI success or formal PR acceptance.
Tested on
Environment (optional)
The Ink/headless matrix used Node 25.9.0; strict OpenTUI used Bun 1.3.13; separate test-engineer checks used Node 24.14.1; packaged CLI with isolated HOME/workspace, fake provider, real native child processes, sandbox disabled. No live model service was used.
Risk & Scope
Linked Issues
Related to #13749 (P02b slice; does not close the umbrella issue).
中文说明
本 PR 的改动
P02b 将宿主和 worker 两侧的 60 秒 Mod 工具根限额改为累计计费时间。仅在可信桥确认 guest 工作空闲时，排除真实原生执行和审批等待。guest 检查、续体、jobs 和日志观察者仍计费；新增 attempt 不刷新余额。root/phase 身份拒绝过期活动消息，取消保留独立的 250ms worker watchdog。
必需的取消 fixture 还发现了 Ink 原有的结果配对问题：Esc 已停止原生进程并记录取消，下一轮却收到“结果缺失”占位。最小修正使普通取消会话也将最终真实响应配对到历史，包括成功与取消混合批次。Review 还发现异步结果落盘可跨越
/clear：完成回调现在在统计/记录前重核原 session 和 chat，释放过期的本地 Goal 状态，并跳过已经配对的响应。用户文档和既有 no-AK E2E 矩阵同步说明并验证新行为。原因
原先两个外层时钟都把原生等待当作 Mod 执行。因此，插件空闲时，合法的长工具执行或未回答审批仍可能在约 60 秒被取消。只暂停一侧或增大限额，都不能修正计费与取消边界。
Reviewer Test Plan
验证行为
前后证据
Before：已安装的 0.25.3 CLI 在 Mods 开启时于 60059.7ms 取消原生 oracle；关闭对照在 66001.9ms 完成。另一个固定 base 的 Ink 构建在约 61 秒丢失审批，也复现了取消结果占位问题。这些是分别记录的不同基线。
After：原预算候选
efc5928d8b的 41 个真实产物场景全部首次通过（Ink 16 项 + headless 9 项；严格 OpenTUI 16 项），零重试。两端审批实际等待 65.098s / 65.077s 后批准成功；Ink、OpenTUI、headless 前台原生执行分别为 66.011s / 66.007s / 66.028s。65 秒采样时均未提前结算；原生 timeout、取消、结果配对、NEXT 和退出检查通过，产物前后 hash 一致。修复提交
63c4956b2014994050e29883ec04f061f0fd8d50：最终提交的 Ink terminal 16/16 首次通过、零重试；长审批 65.063s 后批准成功，前台原生执行同 PID 持续 66.029s，65s 采样时尚未结算。原生 5s timeout、执行中取消、一次结果/记录、NEXT 和自然退出均通过。独立探针对原 efc hook 为 1/1 红、最终 source 为 1/1 绿（均真实落盘 220000 字节；修复后新 chat 及新记录无旧结果）；最终 bundle 的独立真实 Ink 取消验证也通过，产物前后 hash 一致。修复提交验证：完整 build、typecheck、bundle；310 项流式 hook、29 项 clear/scheduler 边界测试和改动文件 ESLint/Prettier。原预算提交还通过了 130 项 core、159 项 CLI 边界和 1 项 relocated-worker 打包契约测试；这些未改动组件的结果保留原提交归属。真实 worker 单测证明活动转换，累计耗尽由确定性的账本/协议测试证明，真实长等待另由 E2E 验证。独立测试工程师复现了原问题，并分别复验长等待候选及最终 Ink 取消。最终 hash 和命令见独立 E2E 报告评论。本地验证和源码审查不代表远端 CI 成功或正式 PR 验收通过。
测试平台
环境
Ink/headless 矩阵使用 Node 25.9.0；严格 OpenTUI 使用 Bun 1.3.13；独立测试工程师检查使用 Node 24.14.1；构建后的 CLI、隔离 HOME/工作目录、假模型服务、真实原生子进程、关闭 sandbox。未使用线上模型服务。
风险与范围
关联问题
关联 #13749（P02b 切片，不关闭总议题）。