fix(core): address post-merge monitor tool and UI routing issues#3792
Conversation
- Guard token bucket against clock drift after system suspend/resume (negative elapsed resets lastRefill instead of starving the bucket) - Add debugLogger.warn for AST read-only check failures in monitor getConfirmationDetails (previously silent catch) - Consolidate SHELL_TOOL_NAMES: export from rule-parser.ts, import in permission-manager.ts (removes identical SHELL_LIKE_TOOLS duplicate) - Extract hasBlockingBackgroundWork/resetBackgroundStateForSessionSwitch to shared backgroundWorkUtils.ts (removes identical copies in clearCommand.ts and useResumeCommand.ts) - Consolidate getToolCallComponent routing into packages/webui (removes near-identical copies in ChatViewer.tsx and vscode-ide-companion, adds missing web_search compat alias to VSCode path) - Add test for droppedLines count in terminal notification text - Add test for exit(null, null) settlement (externally killed process) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
There was a problem hiding this comment.
Pull request overview
Follow-up fixes after the monitor tool merge, addressing reliability issues (clock drift + missing diagnostics), consolidating duplicated constants/utilities, and de-duplicating tool-call UI routing between WebUI and the VSCode webview.
Changes:
- Harden monitor stdout throttling against system clock going backwards; add missing warn logs on AST parsing failures; add/extend unit tests around monitor settlement + notifications.
- Consolidate shared logic/constants (shell-like tool set; background work blocking/reset helpers) to avoid duplication and drift.
- Extract shared
getToolCallComponentrouting into@qwen-code/webuiand reuse it from both ChatViewer and VSCode (including aweb_searchalias).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/webui/src/index.ts | Re-exports getToolCallComponent from the toolcalls package for downstream consumers. |
| packages/webui/src/components/toolcalls/routing.ts | New shared tool-call routing function used by both WebUI and VSCode toolcall renderers. |
| packages/webui/src/components/toolcalls/index.ts | Exposes the new shared router from the toolcalls barrel export. |
| packages/webui/src/components/ChatViewer/ChatViewer.tsx | Removes in-file routing logic in favor of shared getToolCallComponent. |
| packages/vscode-ide-companion/src/webview/components/messages/toolcalls/index.tsx | Switches VSCode webview toolcall routing to use @qwen-code/webui’s shared router. |
| packages/vscode-ide-companion/src/webview/components/messages/toolcalls/index.test.tsx | Updates mocks to align with importing getToolCallComponent from @qwen-code/webui. |
| packages/core/src/tools/monitor.ts | Adds AST failure warning log and token-bucket clock-drift guard. |
| packages/core/src/tools/monitor.test.ts | Adds coverage for exit(null, null) / close(null, null) settling behavior and terminal notification output. |
| packages/core/src/services/monitorRegistry.test.ts | Adds coverage asserting dropped-line counts are included in terminal notification text. |
| packages/core/src/permissions/rule-parser.ts | Exports SHELL_TOOL_NAMES for reuse by permission manager (dedup). |
| packages/core/src/permissions/permission-manager.ts | Reuses SHELL_TOOL_NAMES from rule-parser (removes duplicate set). |
| packages/cli/src/ui/utils/backgroundWorkUtils.ts | New shared background-work helpers extracted from duplicated command/hooks code. |
| packages/cli/src/ui/hooks/useResumeCommand.ts | Uses shared background-work helpers (removes duplication). |
| packages/cli/src/ui/commands/clearCommand.ts | Uses shared background-work helpers (removes duplication). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Conflict in monitorRegistry.test.ts: both PR #3792 (droppedLines test) and PR #3791 (setStatusChangeCallback tests) added tests at the same location. Both sets of tests are preserved. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
- Add debugLogger.warn for clock-drift guard (observability for throttle bucket resets after suspend/resume) - Add test for clock-drift recovery (elapsed < 0 bucket reset) - Add test for AST parse failure catch path (mockRejectedValueOnce) - Forward isFirst/isLast props through VSCode ToolCallRouter (fixes timeline connector rendering) - Add test for shell running branch in hasBlockingBackgroundWork 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix React.FC missing import: use `import type { FC } from 'react'`
instead of `React.FC` (original file had this import before refactor)
- Tighten clock-drift test: emit while clock is in the past to confirm
guard resets lastRefill, then verify refill at the new reference point
🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
此 PR 主要进行代码整理和防御性修复:提取共享工具函数、合并 tool-call 路由、Monitor 时钟回退保护和 AST 失败日志。无关键问题。
未锚定到具体 diff 行的发现
routing.ts缺少测试文件 (Suggestion):getToolCallComponent覆盖约 27 种 kind 值但没有测试文件。建议添加routing.test.ts覆盖每种路由类别。isFirst/isLast属性转发缺少测试 (Suggestion):ToolCallRouter现在传递这些属性给子组件,但 VSCode companion 测试中未验证转发行为。
— deepseek-v4-pro via Qwen Code /review
- Type SHELL_TOOL_NAMES as ReadonlySet<string> to prevent accidental mutation of permission-critical set - Use BackgroundShellRegistry.hasRunningEntries() instead of getAll().some() for zero-allocation short-circuit check - Update clearCommand test mocks to include hasRunningEntries 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
- Merge AgentToolCall + isAgentExecutionToolCall into single import in routing.ts (comment 3178280762) - Use real getToolCallComponent via vi.importActual in VSCode test mock so routing logic is validated, not a parallel mock that can drift (comment 3178280775) - Validate isFirst/isLast forwarding in VSCode test mock via data attributes (comment 3178346891) - Add comment documenting debugLogger.warn no-op tradeoff for clock drift guard (comment 3178346889) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
left a comment
There was a problem hiding this comment.
[Critical] packages/cli/src/ui/hooks/useResumeCommand.test.ts — Stale mock after backgroundWorkUtils extraction
The PR extracted hasBlockingBackgroundWork into backgroundWorkUtils.ts, changing the implementation from getAll().some(entry => entry.status === 'running') to hasRunningEntries(). However, useResumeCommand.test.ts was not updated — all four getBackgroundShellRegistry mocks still provide getAll and omit hasRunningEntries.
Existing tests pass only because hasBlockingBackgroundWork short-circuits on earlier || conditions (background tasks or monitors). Adding a test that exercises the background shell path (analogous to the new test in clearCommand.test.ts) will throw TypeError: config.getBackgroundShellRegistry(...).hasRunningEntries is not a function.
Fix: Add hasRunningEntries: vi.fn().mockReturnValue(false) to all getBackgroundShellRegistry mocks in useResumeCommand.test.ts (matching the pattern already used in clearCommand.test.ts).
wenshao
left a comment
There was a problem hiding this comment.
Review Summary
Post-merge followup that consolidates duplicated code (SHELL_TOOL_NAMES, backgroundWorkUtils, getToolCallComponent routing), fixes a clock-drift token-bucket guard in the monitor tool, and adds logging to a previously-silent catch block. Test coverage is solid with 203 passing tests across changed packages.
No Critical issues. All Suggestion-level findings from this review round were already raised and addressed in prior review rounds (see inline comment threads).
— deepseek-v4-pro via Qwen Code /review
Covers all 8 component branches including the web_search compatibility alias, agent execution detection, case-insensitive matching, and fallback to GenericToolCall. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The backgroundWorkUtils refactor replaced getAll().some() with hasRunningEntries(), but the test mocks in useResumeCommand.test.ts were not updated, causing CI failures. 🤖 Generated with [Qoder Code](https://github.com/QwenLM/qwen-code)
Cover hasBlockingBackgroundWork (6 cases including short-circuit behaviour) and resetBackgroundStateForSessionSwitch (1 case verifying all three registries are reset). 🤖 Generated with [Qoder Code](https://github.com/QwenLM/qwen-code)
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] packages/cli/src/ui/hooks/useResumeCommand.test.ts — hasBlockingBackgroundWork 已提取为使用 hasRunningEntries(),mock 也已补上该方法,但缺少后台 shell 运行时的阻塞路径测试(hasRunningEntries: true)。clearCommand.test.ts 第 477-513 行有对应的测试。建议仿照添加。
— deepseek-v4-pro via Qwen Code /review
| * Shell tool canonical names. These use command-style rule specifiers. | ||
| */ | ||
| const SHELL_TOOL_NAMES = new Set(['run_shell_command', 'monitor']); | ||
| export const SHELL_TOOL_NAMES: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
[Suggestion] SHELL_TOOL_NAMES 导出时 JSDoc 过于简略,丢失了原来 permission-manager.ts 中 SHELL_LIKE_TOOLS 的详细注释(复合命令拆分、AST 只读分析等)。建议补充:
| export const SHELL_TOOL_NAMES: ReadonlySet<string> = new Set([ | |
| /** | |
| * Shell tool canonical names. These tools spawn shell commands and share | |
| * the same permission evaluation semantics: compound-command splitting, | |
| * AST read-only analysis, and virtual file/network operation matching. | |
| */ | |
| export const SHELL_TOOL_NAMES: ReadonlySet<string> = new Set([ | |
| 'run_shell_command', | |
| 'monitor', | |
| ]); |
— deepseek-v4-pro via Qwen Code /review
|
|
||
| import type { Config } from '@qwen-code/qwen-code-core'; | ||
|
|
||
| export function hasBlockingBackgroundWork(config: Config): boolean { |
There was a problem hiding this comment.
[Suggestion] 提取到共享模块的 hasBlockingBackgroundWork 缺少 JSDoc。建议添加说明其检查顺序和阻塞语义:
| export function hasBlockingBackgroundWork(config: Config): boolean { | |
| /** | |
| * Returns true when any background work is still in progress that would | |
| * conflict with starting or resuming a session. | |
| * | |
| * Checks in short-circuit order: unfinalized tasks, running monitors, | |
| * then running background shells. | |
| */ | |
| export function hasBlockingBackgroundWork(config: Config): boolean { |
— deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
复审通过 (post-iteration)
CI 已通过,前两轮迭代(607bd809c / 57d089c)的 review 项基本闭环。重新通读 end-to-end,修复都到位:
monitor.ts时钟回退保护正确且保守(重置lastRefill但不补 token 桶,避免唤醒后突发);rewritten 测试monitor.test.ts:1303切实触发了elapsed < 0分支- AST 解析失败的
debugLogger.warn与同文件getDefaultPermission的模式一致;sub-command 仍留在 confirmation scope SHELL_TOOL_NAMES整合干净,无残留SHELL_LIKE_TOOLS引用;ReadonlySet<string>类型签名也加上了backgroundWorkUtils中hasRunningEntries()严格优于getAll().some(...)(无中间数组拷贝、语义等价)routing.ts是两份原副本的并集;VSCode 路由获得web_searchlegacy alias 作为严格超集- VSCode 测试 mock 改用真实
getToolCallComponent+ label 映射的方式相比之前的 parallel mock router 更稳,能捕获路由漂移
附两条非阻断性的 forward-looking 建议(inline)。
| if (newTokens > 0) { | ||
| tokenBucket = Math.min(THROTTLE_BURST_SIZE, tokenBucket + newTokens); | ||
| lastRefill += newTokens * THROTTLE_REFILL_INTERVAL_MS; | ||
| if (elapsed < 0) { |
There was a problem hiding this comment.
[Suggestion / future] Date.now() 本质是非单调时钟(wall clock),这正是需要 elapsed < 0 保护的根本原因。更彻底的方案是把 throttle 时钟切到 performance.now()(Node 18+ 全局可用,单调递增,免疫 suspend/resume 与 NTP 回退),这段保护代码就可以彻底删掉。
本 PR 不必处理,标记给后续。
| ) => { | ||
| const realComponent = realGetToolCallComponent(toolCall); | ||
| const componentName = realComponent.displayName || realComponent.name || ''; | ||
| return componentMocks[componentName] || componentMocks['GenericToolCall']!; |
There was a problem hiding this comment.
[Suggestion] componentMocks[componentName] || componentMocks['GenericToolCall']! 的回退会静默掩盖查找失败。如果未来任何一个 ToolCall 组件被 React.memo() 或 forwardRef() 包装,realComponent.name 会变成 '' / 'memo' / 'ForwardRef' 等,测试会悄悄走 GenericToolCall 分支而不报错——routing drift 又会被遮住。
建议查不到时直接抛错(防御性更强):
const realComponent = realGetToolCallComponent(toolCall);
const componentName = realComponent.displayName || realComponent.name || '';
const mock = componentMocks[componentName];
if (!mock) {
throw new Error(`No mock registered for component '${componentName}'`);
}
return mock;这样 PR 改组件命名/包装时测试会立刻喊出来。
wenshao
left a comment
There was a problem hiding this comment.
LGTM. CI 已绿,前两轮 review 项已闭环。
* fix(core): address post-merge monitor tool and UI routing issues - Guard token bucket against clock drift after system suspend/resume (negative elapsed resets lastRefill instead of starving the bucket) - Add debugLogger.warn for AST read-only check failures in monitor getConfirmationDetails (previously silent catch) - Consolidate SHELL_TOOL_NAMES: export from rule-parser.ts, import in permission-manager.ts (removes identical SHELL_LIKE_TOOLS duplicate) - Extract hasBlockingBackgroundWork/resetBackgroundStateForSessionSwitch to shared backgroundWorkUtils.ts (removes identical copies in clearCommand.ts and useResumeCommand.ts) - Consolidate getToolCallComponent routing into packages/webui (removes near-identical copies in ChatViewer.tsx and vscode-ide-companion, adds missing web_search compat alias to VSCode path) - Add test for droppedLines count in terminal notification text - Add test for exit(null, null) settlement (externally killed process) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,cli,vscode): address PR #3792 review feedback - Add debugLogger.warn for clock-drift guard (observability for throttle bucket resets after suspend/resume) - Add test for clock-drift recovery (elapsed < 0 bucket reset) - Add test for AST parse failure catch path (mockRejectedValueOnce) - Forward isFirst/isLast props through VSCode ToolCallRouter (fixes timeline connector rendering) - Add test for shell running branch in hasBlockingBackgroundWork 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,vscode): address follow-up review comments - Fix React.FC missing import: use `import type { FC } from 'react'` instead of `React.FC` (original file had this import before refactor) - Tighten clock-drift test: emit while clock is in the past to confirm guard resets lastRefill, then verify refill at the new reference point 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,cli): adopt review feedback — ReadonlySet + hasRunningEntries - Type SHELL_TOOL_NAMES as ReadonlySet<string> to prevent accidental mutation of permission-critical set - Use BackgroundShellRegistry.hasRunningEntries() instead of getAll().some() for zero-allocation short-circuit check - Update clearCommand test mocks to include hasRunningEntries 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,webui,vscode): address remaining PR #3792 review comments - Merge AgentToolCall + isAgentExecutionToolCall into single import in routing.ts (comment 3178280762) - Use real getToolCallComponent via vi.importActual in VSCode test mock so routing logic is validated, not a parallel mock that can drift (comment 3178280775) - Validate isFirst/isLast forwarding in VSCode test mock via data attributes (comment 3178346891) - Add comment documenting debugLogger.warn no-op tradeoff for clock drift guard (comment 3178346889) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(webui): add unit test for getToolCallComponent routing Covers all 8 component branches including the web_search compatibility alias, agent execution detection, case-insensitive matching, and fallback to GenericToolCall. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(cli): add missing hasRunningEntries to useResumeCommand test mocks The backgroundWorkUtils refactor replaced getAll().some() with hasRunningEntries(), but the test mocks in useResumeCommand.test.ts were not updated, causing CI failures. 🤖 Generated with [Qoder Code](https://github.com/QwenLM/qwen-code) * test(cli): add unit tests for backgroundWorkUtils shared utility Cover hasBlockingBackgroundWork (6 cases including short-circuit behaviour) and resetBackgroundStateForSessionSwitch (1 case verifying all three registries are reset). 🤖 Generated with [Qoder Code](https://github.com/QwenLM/qwen-code) --------- Co-authored-by: jinye.djy <jinye.djy@alibaba-inc.com>
…nLM#3792) * fix(core): address post-merge monitor tool and UI routing issues - Guard token bucket against clock drift after system suspend/resume (negative elapsed resets lastRefill instead of starving the bucket) - Add debugLogger.warn for AST read-only check failures in monitor getConfirmationDetails (previously silent catch) - Consolidate SHELL_TOOL_NAMES: export from rule-parser.ts, import in permission-manager.ts (removes identical SHELL_LIKE_TOOLS duplicate) - Extract hasBlockingBackgroundWork/resetBackgroundStateForSessionSwitch to shared backgroundWorkUtils.ts (removes identical copies in clearCommand.ts and useResumeCommand.ts) - Consolidate getToolCallComponent routing into packages/webui (removes near-identical copies in ChatViewer.tsx and vscode-ide-companion, adds missing web_search compat alias to VSCode path) - Add test for droppedLines count in terminal notification text - Add test for exit(null, null) settlement (externally killed process) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,cli,vscode): address PR QwenLM#3792 review feedback - Add debugLogger.warn for clock-drift guard (observability for throttle bucket resets after suspend/resume) - Add test for clock-drift recovery (elapsed < 0 bucket reset) - Add test for AST parse failure catch path (mockRejectedValueOnce) - Forward isFirst/isLast props through VSCode ToolCallRouter (fixes timeline connector rendering) - Add test for shell running branch in hasBlockingBackgroundWork 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,vscode): address follow-up review comments - Fix React.FC missing import: use `import type { FC } from 'react'` instead of `React.FC` (original file had this import before refactor) - Tighten clock-drift test: emit while clock is in the past to confirm guard resets lastRefill, then verify refill at the new reference point 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,cli): adopt review feedback — ReadonlySet + hasRunningEntries - Type SHELL_TOOL_NAMES as ReadonlySet<string> to prevent accidental mutation of permission-critical set - Use BackgroundShellRegistry.hasRunningEntries() instead of getAll().some() for zero-allocation short-circuit check - Update clearCommand test mocks to include hasRunningEntries 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(core,webui,vscode): address remaining PR QwenLM#3792 review comments - Merge AgentToolCall + isAgentExecutionToolCall into single import in routing.ts (comment 3178280762) - Use real getToolCallComponent via vi.importActual in VSCode test mock so routing logic is validated, not a parallel mock that can drift (comment 3178280775) - Validate isFirst/isLast forwarding in VSCode test mock via data attributes (comment 3178346891) - Add comment documenting debugLogger.warn no-op tradeoff for clock drift guard (comment 3178346889) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(webui): add unit test for getToolCallComponent routing Covers all 8 component branches including the web_search compatibility alias, agent execution detection, case-insensitive matching, and fallback to GenericToolCall. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(cli): add missing hasRunningEntries to useResumeCommand test mocks The backgroundWorkUtils refactor replaced getAll().some() with hasRunningEntries(), but the test mocks in useResumeCommand.test.ts were not updated, causing CI failures. 🤖 Generated with [Qoder Code](https://github.com/QwenLM/qwen-code) * test(cli): add unit tests for backgroundWorkUtils shared utility Cover hasBlockingBackgroundWork (6 cases including short-circuit behaviour) and resetBackgroundStateForSessionSwitch (1 case verifying all three registries are reset). 🤖 Generated with [Qoder Code](https://github.com/QwenLM/qwen-code) --------- Co-authored-by: jinye.djy <jinye.djy@alibaba-inc.com>
Summary
Follow-up to PR #3684 (monitor tool) addressing review items that were not resolved before merge, plus a UI routing duplication fix from the agent tool display feature.
Date.now()can jump backward, makingelapsednegative and starving the token bucket. Added a guard that resetslastRefillwhen the clock goes backwards.getConfirmationDetailscatch block was completely silent. AddeddebugLogger.warnmatching the pattern already used ingetDefaultPermissionin the same file.SHELL_TOOL_NAMES: IdenticalSet(['run_shell_command', 'monitor'])was defined twice with different names (SHELL_LIKE_TOOLS/SHELL_TOOL_NAMES). Now exported once fromrule-parser.ts(typed asReadonlySet<string>) and imported inpermission-manager.ts.hasBlockingBackgroundWork()andresetBackgroundStateForSessionSwitch()were duplicated inclearCommand.tsanduseResumeCommand.ts. Extracted to a shared module, also replacinggetAll().some()withhasRunningEntries()for a zero-allocation short-circuit check (semantically equivalent).getToolCallComponentrouting: Near-identical tool-call routing logic existed in bothChatViewer.tsxand the VSCode toolcalls router. Extracted to a sharedrouting.tsin@qwen-code/webui. Also adds the missingweb_searchcompatibility alias to the VSCode path.droppedLinescount in terminal notification text (I7),exit(null, null)settlement behavior (I8), clock-drift guard recovery, AST parse failure handling, andisFirst/isLastprop forwarding. Addedrouting.test.tscovering all 8 component branches includingweb_search.Reviewer Test Plan
/clearand/resumecommands still block when background work is runningrun_shell_command,monitor)🤖 Generated with Qwen Code