feat: add tool execution engine with diff review, inline highlighting and per-round tool steps

[genni613]

User description

Summary

• 新增完整的工具执行引擎（ToolCallDispatcher / ToolRegistry / 各 Executor），统一管理 Bash、Read、Write、Edit、Grep、ListFiles 等工具的调度与执行
• 实现 Diff 审查机制（DiffHunk / ChangeReviewStrategy / DialogReview / EditorInlineReview），支持对话弹窗和编辑器内联两种审查模式
• 前端新增分轮次工具步骤展示（ToolStepsBar / ToolStepRow）和文件变更内联高亮（FileChangeInline），提升工具调用过程可视化
• 引入 MessageGroup 模型替代扁平 Message[]，支持按轮次分组展示 Assistant 消息
• 将 15 个 Bridge 回调统一为单一 onEvent 通道，并新增 eventReducer / groupReducer 处理前端事件状态

Changes

• 后端 - 工具执行引擎: 新增 execution/ 包，含 ToolCallDispatcher、ToolRegistry、6 个
  Executor（Bash/EditFile/GrepFiles/ListFiles/ReadFile/WriteFile）、PermissionMode、FileWriteLock、PostEditPipeline 等
• 后端 - Diff 审查: 新增 DiffHunk 解析、ChangeReviewStrategy 策略模式、DialogReview 对话审查、EditorInlineReview 编辑器内联审查、FileChangeReview 数据模型
• 后端 - Bridge/ChatService 重构: BridgeHandler 与 ChatService 适配新引擎，统一 onEvent 事件通道
• 前端 - 组件: 新增 AssistantGroup、AssistantMarkdown、ToolStepRow、ToolStepsBar、FileChangeInline 组件
• 前端 - 状态管理: 新增 eventReducer（事件处理）和 groupReducer（消息分组），替代原有 App.tsx 中的内联逻辑
• 前端 - 类型/样式: 更新 bridge.d.ts 类型定义，新增 App.css 样式
• 测试: 新增 eventReducer.test.mjs 和 groupReducer.test.mjs 单元测试
• 设计文档: 更新 unified-tool-design.md，新增 editor-inline-review-design.md 和 tool-step-visualization.md

Test plan

• 验证工具执行引擎能正确调度 Bash/Read/Write/Edit/Grep/ListFiles 工具并返回结果
• 验证 Diff 审查流程：对话弹窗模式能正确展示变更并支持接受/拒绝
• 验证编辑器内联审查模式能高亮显示文件变更
• 验证前端 ToolStepsBar 正确按轮次展示工具调用步骤
• 验证 MessageGroup 分组渲染 Assistant 消息正常
• 验证 onEvent 统一事件通道各事件类型正确传递与处理
• 运行 eventReducer / groupReducer 单元测试全部通过

---

PR Type

Enhancement

---

Description

• Unify 15 bridge callbacks into single onEvent channel with eventReducer for frontend state management

• Add complete tool execution engine (ToolCallDispatcher, ToolRegistry, 6 Executors) with permission modes and review strategies

• Implement diff review mechanism with EditorInlineReview (IntelliJ native diff dialog) and DialogReview fallback

• Add new settings for unified tools: permissionMode, unifiedToolsEnabled, reviewMode, allowSessionTrust

---

Diagram Walkthrough

flowchart LR
  A[Kotlin Backend] -->|15 callbacks| B[BridgeHandler]
  B -->|single onEvent| C[window.__bridge.onEvent]
  C -->|dispatch| D[eventReducer]
  D --> E[AppState]
  
  F[ToolCallDispatcher] --> G[ToolRegistry]
  G --> H[6 Executors]
  H --> I[ChangeReviewStrategy]
  I --> J[EditorInlineReview/DialogReview]

Loading

File Walkthrough

	Relevant files
Enhancement	29 files BridgeHandler.kt Unify 15 callbacks into single onEvent channel                      +107/-59 ToolCallDispatcher.kt Add unified tool dispatcher with full pipeline                      +605/-0  ChatService.kt Integrate unified tool system in ChatService                          +143/-43 ToolSpecs.kt Define 6 built-in tool specifications                                        +225/-0  ToolRegistry.kt Add central tool registry with lifecycle management            +79/-0    EditorInlineReview.kt Implement IntelliJ native diff dialog review                          +167/-0  DialogReview.kt Add fallback dialog-based review strategy                                +91/-0    ChangeReviewStrategy.kt Define review strategy interface                                                  +42/-0    DiffHunk.kt Implement LCS-based diff calculation                                          +146/-0  BashExecutor.kt Add bash executor with dynamic permission classification  +122/-0  EditFileExecutor.kt Add file edit executor with pending review                              +102/-0  ReadFileExecutor.kt Add file read executor with line pagination                            +86/-0    WriteFileExecutor.kt Add file write executor with pending review                            +62/-0    GrepFilesExecutor.kt Add grep/search executor                                                                  +92/-0    ListFilesExecutor.kt Add directory listing executor                                                      +54/-0    FileChangeReview.kt Manage file change review with session trust                          +131/-0  ToolResult.kt Define unified result type for all tools                                  +42/-0    PermissionMode.kt Define permission level enum                                                          +17/-0    ToolSpec.kt Define tool registration info structure                                    +21/-0    ToolExecutor.kt Define tool executor interface                                                      +12/-0    ToolContext.kt Define execution context for tools                                              +13/-0    ToolHook.kt Define hook extension point for tools                                        +25/-0    ToolExecutionLogger.kt Add default logging hook implementation                                    +25/-0    FileWriteLock.kt Add file-level write lock for serialization                            +27/-0    PostEditPipeline.kt Add post-edit quality pipeline stub                                            +35/-0    InlineChangeHighlighter.kt Add inline change highlighter stub                                              +34/-0    ExecutionResult.kt Add toToolResult conversion method                                              +15/-0    eventReducer.ts Add unified event reducer for state management                      +227/-0  bridge.d.ts Update bridge types for single onEvent channel                      +4/-15
Configuration changes	1 files PluginSettings.kt Add unified tool system settings                                                  +7/-1
Additional files	20 files editor-inline-review-design.md +751/-0  tool-step-visualization.md +433/-0  unified-tool-design.md +1461/-209 2026-04-19-phase1-unified-event-channel-design.md +391/-0  2026-04-19-phase2-message-grouping-design.md +488/-0  index.html +73/-71  package.json +1/-1      App.css +166/-0  App.tsx +72/-204 AssistantGroup.tsx +85/-0    AssistantMarkdown.tsx +83/-0    FileChangeInline.tsx +40/-0    MessageBubble.tsx +6/-7      ToolStepRow.tsx +77/-0    ToolStepsBar.tsx +106/-0  groupReducer.ts +299/-0  useBridge.ts +13/-75  eventReducer.test.mjs +262/-0  groupReducer.test.mjs +436/-0  tsconfig.test.json +4/-2

---

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 Security concerns Dangerous command bypass: The deny rule patterns for detecting dangerous rm commands (both in ToolCallDispatcher.kt line 479 and BashExecutor.kt lines 104-105) have a flaw - they require a space before the path separator. This means rm -rf / will bypass the dangerous delete detection, allowing a potentially catastrophic root-directory delete. The pattern should be rm\s+(-\w*\s*)*(-r|--recursive).*\s+(/|~|$) or similar to catch paths that start with / or ~ without preceding whitespace. The whitelist being ignored when unifiedToolsEnabled is true is also a security concern if users depend on the whitelist for blocking certain commands.

⚡ Recommended focus areas for review Dangerous delete bypass The deny rule regex on line 479 only matches commands like rm -rf /home or rm -rf ~ (with a space before the path). It will NOT match rm -rf / or rm -rf /* because the regex requires whitespace before the / or ~ character. This creates a serious security bypass where a destructive root-level delete could slip through the pre-execution denial check. if (Regex("""rm\s+(-\w*\s*)*(-r|--recursive).*\s+(/|~)""", RegexOption.IGNORE_CASE).containsMatchIn(cmd)) return "Dangerous delete command detected" Dangerous delete bypass Same issue as in ToolCallDispatcher: the DANGEROUS_DELETE_PATTERN on lines 104-105 requires whitespace before the path separator, so commands like rm -rf / bypass the check. This affects both pre-execution denial in dispatch and runtime checks in the executor itself. private val DANGEROUS_DELETE_PATTERN = Regex("""rm\s+(-\w*\s*)*(-r|--recursive).*\s+(/|~)""", RegexOption.IGNORE_CASE) Unclear whitelist interaction When unifiedToolsEnabled is true, the command whitelist is not respected (lines 262-268). The code explicitly notes this with a comment saying "unified dispatcher doesn't use command-level whitelist the same way." This means users who have configured a command whitelist expecting certain commands to be blocked will find the whitelist is ignored when the unified tool system is enabled. if (PluginSettings.getInstance().getState().unifiedToolsEnabled) { if (addToWhitelist && decision == "allow") { // Note: unified dispatcher doesn't use command-level whitelist the same way. // For now, also handle via legacy path for backwards compatibility. } dispatcher.onApprovalResponse(requestId, decision) }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove unresolved merge conflict markers Unresolved merge conflict markers are present in the document. These must be resolved before merging - choose either "Updated upstream" (v3.0) or "Stashed changes" (v3.3), or manually merge both versions if both contain needed changes. docs/design/unified-tool-design.md [3-7] -+<<<<<<< Updated upstream -+> **版本**: v3.0 -+======= -+> **版本**: v3.3 -+>>>>>>> Stashed changes +> **版本**: v3.3 Suggestion importance[1-10]: 8 __ Why: The document contains unresolved merge conflict markers (<<<<<<<, =======, >>>>>>>) which would corrupt the markdown rendering. This is a critical issue that must be resolved before merging. The suggestion correctly identifies the problem and provides an appropriate fix.	Medium
	Fix round_end case capturing stale state in filter closure The round_end case references state.currentRoundTextIndex inside the updater function after it's already been set to null in the spread. This causes incorrect filtering since the closure captures the old state but the filter runs with null. Also, state.currentRoundTextIndex is null at this point, so the filter condition is broken. webview/src/groupReducer.ts [177-188] case 'round_end': { - if (state.currentRoundTextIndex !== null) { + const idx = state.currentRoundTextIndex + if (idx !== null) { return updateLastAssistant({ ...state, currentRoundTextIndex: null, }, group => ({ ...group, - children: group.children.filter((_, i) => i !== state.currentRoundTextIndex), + children: group.children.filter((_, i) => i !== idx), })) } return state } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that state.currentRoundTextIndex is referenced inside the callback after being set to null in the spread, which could cause the filter to use an incorrect value. The fix captures the index in a local variable before nulling it. This is a valid bug fix, though not critical since the callback runs synchronously.	Medium
	Implement diff statistics extraction or remove dead code The extractDiffStats method always returns null even when the tool is edit_file or write_file. This method should compute and return diff statistics (additions/deletions) for the tool result, but the implementation is missing. Either implement the diff calculation or remove this dead code. src/main/kotlin/com/github/codeplangui/execution/ToolCallDispatcher.kt [416-419] private fun extractDiffStats(toolName: String, input: JsonObject, result: ToolResult): String? { if (toolName != "edit_file" && toolName != "write_file") return null - return null + if (!result.ok || result.pendingReview == null) return null + val review = result.pendingReview + val oldLines = review.originalContent.lines().size + val newLines = review.newContent.lines().size + val diff = newLines - oldLines + return if (diff >= 0) "+$diff" else "$diff" } Suggestion importance[1-10]: 6 __ Why: The extractDiffStats method always returns null even when the tool is edit_file or write_file, making it dead code. The improved implementation correctly calculates diff statistics (additions/deletions) from the pending review data and returns them in the expected format (+N or -N). This is a valid fix for incomplete code, though not a critical bug since it only affects optional diff statistics reporting.	Low
General	Verify section reference numbers are accurate The table shows MCP tools reference §10 but Skills reference §11, yet the document structure shows Skills section is labeled as M6 (section 11 in the document). Ensure section numbering is consistent throughout - either update the reference to match the actual section number or use a more generic reference. docs/design/unified-tool-design.md [66-67] -+| MCP 工具 | `mcp__*__*` | 需审批（默认） | 动态加载（见 §10） | -+| Skills | `execute_skill` | 安全属性自动放行 | 多步技能编排（见 §11） | +| MCP 工具 | `mcp__*__*` | 需审批（默认） | 动态加载（见 §10） | +| Skills | `execute_skill` | 安全属性自动放行 | 多步技能编排（见 §15） | Suggestion importance[1-10]: 3 __ Why: The suggestion correctly points out that Skills references §11 while being in section 11, but in the new hunk this appears correct. The improved_code suggestion to change to §15 is incorrect since the Skills section is indeed at section 11 (M6 in the table at lines 150-151). The suggestion has a minor validity issue with the proposed fix but correctly identifies potential numbering confusion in the original code.	Low