feat(tools): M2+M3 — ToolRegistry, FileReadTool, FileEditTool, real permission callback

[TuYv]

User description

Summary

• M2: ToolRegistry (static pool management), FileReadTool (offset/limit pagination, 2MB cap, workspace path check), runToolUseBatch (safe-parallel / unsafe-serial partition)
• Bugfix: replaced limitedParallelism(n) with Semaphore(n) — the former limits threads, not coroutines; a suspended tool releases its thread and bypasses the bound
• M3: FileEditTool (string replacement + unified diff preview, Ask in DEFAULT mode / Allow in ACCEPT_EDITS/BYPASS), onPermissionAsked suspend callback in ToolExecutionContext, runToolUse now suspends on Ask and emits Failed(PERMISSION) on denial

Test plan

• ToolExecutorBatchTest (8 tests): empty batch, LOOKUP failure, safe parallel, unsafe serial, mixed, maxConcurrency bound, parse isolation, per-id ordering
• ToolRegistryTest (8 tests): base tools include Bash/FileEdit/FileRead, assembleToolPool sort+dedupe, deny rules exact+parametrized, findByName by name+alias
• FileEditToolTest (22 tests): validate, workspace path guard, oldString-not-found deny, Ask/Allow per mode, preview diff, call replaceFirst/replaceAll/write, metadata flags, permission denied via callback, PermissionAsked event ordering, buildUnifiedDiff unit tests

All 38 tests pass.

🤖 Generated with Claude Code

---

PR Type

Enhancement, Bug fix

---

Description

• Add ToolRegistry with getAllBaseTools, filterToolsByDenyRules, assembleToolPool, and findByName

• Add FileReadTool with offset/limit pagination, 2MB byte cap, and workspace path validation

• Add FileEditTool with string replacement and unified diff preview for permission Ask mode

• Fix concurrency bug: replace limitedParallelism with Semaphore for correct coroutine-level throttling

• Add real permission callback (onPermissionAsked) to ToolExecutionContext that suspends on Ask

• Add runToolUseBatch for safe-parallel / unsafe-serial multi-tool execution

---

Diagram Walkthrough

flowchart LR
  A[ToolUseBatch] --> B{Classify by isConcurrencySafe}
  B -->|Safe| C[Semaphore Parallel]
  B -->|Unsafe| D[Serial Execution]
  C --> E[Tool Updates Flow]
  D --> E
  F[Permission Check] --> G{Result}
  G -->|Ask| H[onPermissionAsked Callback]
  G -->|Allow| I[Execute Tool]
  H -->|Allowed| I
  H -->|Denied| J[Failed(PERMISSION)]

Loading

File Walkthrough

	Relevant files
Enhancement	5 files ToolExecutionContext.kt Add onPermissionAsked suspend callback                                      +7/-0      ToolExecutor.kt Add runToolUseBatch and fix concurrency                                    +124/-2  ToolRegistry.kt Create ToolRegistry for tool pool management                          +74/-0    FileEditTool.kt Create FileEditTool with diff preview                                        +241/-0  FileReadTool.kt Create FileReadTool with pagination                                            +198/-0
Tests	4 files ToolExecutorBatchTest.kt Add batch executor tests                                                                  +245/-0  ToolRegistryTest.kt Add ToolRegistry tests                                                                      +103/-0  FileEditToolTest.kt Add FileEditTool tests                                                                      +274/-0  FileReadToolTest.kt Add FileReadTool tests                                                                      +168/-0

---

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review TOCTOU race condition Between checkPermissions (lines 96-99) and call (lines 151-153), the file content could change. The code checks that oldString exists during permission check, but re-checks and re-counts during execution. If the file is modified externally between these two points, the second count could differ from the first, potentially causing unexpected behavior or errors. The call block uses require(count > 0) which would throw, but a more graceful handling might be expected. val count = content.split(input.oldString).size - 1 if (count == 0) { return@checkPermissions PermissionResult.Deny("oldString not found in ${input.path}") } // ACCEPT_EDITS / BYPASS modes skip the confirmation dialog if (ctx.permissionContext.mode == ToolPermissionContext.Mode.ACCEPT_EDITS || ctx.permissionContext.mode == ToolPermissionContext.Mode.BYPASS) { return@checkPermissions PermissionResult.Allow( Json.encodeToJsonElement(FileEditInput.serializer(), input) ) } val effectiveCount = if (input.replaceAll) count else 1 val diff = buildEditPreviewDiff(content, input.oldString, input.newString, input.replaceAll, input.path) PermissionResult.Ask( reason = "Edit ${input.path}: replace $effectiveCount occurrence(s)", preview = PreviewResult( summary = "Replace $effectiveCount occurrence(s) in ${input.path}", details = diff, risk = PreviewResult.Risk.MEDIUM, ), ) } preview { input, ctx -> val basePath = ctx.project.basePath ?: return@preview null val resolved = resolveInsideWorkspace( input.path, basePath, ctx.permissionContext.additionalWorkingDirectories ) ?: return@preview null val content = withContext(Dispatchers.IO) { val f = File(resolved) if (!f.exists() || !f.isFile) null else f.readText(Charsets.UTF_8) } ?: return@preview null val count = content.split(input.oldString).size - 1 if (count == 0) return@preview null val effectiveCount = if (input.replaceAll) count else 1 val diff = buildEditPreviewDiff(content, input.oldString, input.newString, input.replaceAll, input.path) PreviewResult( summary = "Replace $effectiveCount occurrence(s) in ${input.path}", details = diff, risk = PreviewResult.Risk.MEDIUM, ) } call { input, ctx, _ -> val basePath = ctx.project.basePath!! val resolved = resolveInsideWorkspace( input.path, basePath, ctx.permissionContext.additionalWorkingDirectories )!! withContext(Dispatchers.IO) { val file = File(resolved) require(file.exists() && file.isFile) { "File not found: ${input.path}" } val original = file.readText(Charsets.UTF_8) val count = original.split(input.oldString).size - 1 require(count > 0) { "oldString not found in ${input.path}" } Misleading replacement count When replaceAll=false, the code returns effectiveCount = 1 (line 109) and reports "replace 1 occurrence(s)" in the permission prompt, but the underlying count variable still reflects the total number of occurrences in the file (lines 96, 152). The permission prompt says "replace 1 occurrence(s)" but internally counts all occurrences. This is potentially confusing to users who approve the permission — they might expect only one replacement based on the prompt, but the code has the full count available. Consider whether the preview should clarify it's replacing only the first match. val effectiveCount = if (input.replaceAll) count else 1 val diff = buildEditPreviewDiff(content, input.oldString, input.newString, input.replaceAll, input.path) PermissionResult.Ask( reason = "Edit ${input.path}: replace $effectiveCount occurrence(s)",

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix inconsistent trailing newline handling There's inconsistent handling of trailing empty lines between the byte-cap branch and the else branch. The byte-cap branch unconditionally drops the last line with .dropLast(1), even if the file doesn't end with a newline. Meanwhile, the else branch correctly checks allLines.last().isEmpty() before dropping. This can cause totalLines to be off by one for large files that don't end with a newline. src/main/kotlin/com/github/codeplangui/tools/file/FileReadTool.kt [113-124] val allLines = if (file.length() > FILE_READ_MAX_BYTES) { val bytes = file.inputStream().buffered().use { it.readNBytes(FILE_READ_MAX_BYTES.toInt()) } - String(bytes, Charsets.UTF_8).lines().dropLast(1) + String(bytes, Charsets.UTF_8).lines() } else { file.readText(Charsets.UTF_8).lines() } -// `readText` + `lines()` produces a trailing empty element for files ending -// in newline. Drop it so totalLines matches intuition. -val totalLines = if (allLines.isNotEmpty() && allLines.last().isEmpty()) allLines.size - 1 else allLines.size +// `lines()` produces a trailing empty element for files ending in newline. +// Drop it so totalLines matches intuition. val effectiveLines = if (allLines.isNotEmpty() && allLines.last().isEmpty()) allLines.dropLast(1) else allLines +val totalLines = effectiveLines.size Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a bug where the byte-cap branch unconditionally drops the last line via .dropLast(1), while the else branch checks if the last line is empty first. For large files that don't end with a newline, this would cause totalLines to be off by one. The fix applies consistent logic to both branches, making the code correct.	Medium