feat(apply-patch): add built-in apply_patch tool#250
Draft
velga111 wants to merge 1 commit into
Draft
Conversation
|
|
github-actions
Bot
added
area:ci
velga111
force-pushed
the
feat/apply-patch-tool
branch
from
3f49275 to
ff964de
Compare
velga111
changed the title
velga111
force-pushed
the
feat/apply-patch-tool
branch
from
ff964de to
de2d551
Compare
hiqiancheng
force-pushed
the
feat/apply-patch-tool
branch
from
de2d551 to
f0acacc
Compare
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR adds a complete ApplyPatch built-in tool to TouchAI desktop, enabling programmatic file mutations via custom patch format. The feature includes Rust core implementation with path validation and hunk matching, Tauri bridging, TypeScript desktop tool with approval/execution, bash file-mutation blocking as a complementary safety feature, database seeding, UI rendering with diff visualization, and comprehensive test coverage across all layers. ChangesApply Patch Built-In Tool
Sequence Diagram(s)sequenceDiagram
participant User as User/Agent
participant Desktop as Desktop App
participant Native as NativeService
participant Tauri as Tauri Backend
participant FileSystem as FileSystem
User->>Desktop: ApplyPatch (patch text, working dir)
Desktop->>Native: native.builtInTools.applyPatch(request)
Native->>Tauri: invoke built_in_tools_apply_patch
Tauri->>Tauri: parse patch format
Tauri->>Tauri: resolve paths, validate safety
Tauri->>Tauri: stage add/update/delete, apply hunks
Tauri->>FileSystem: write files, remove deletions
Tauri->>Tauri: format summary with file changes
Tauri-->>Native: BuiltInApplyPatchExecutionResponse
Native-->>Desktop: response with summary & previews
Desktop->>Desktop: emit displayResult in call_end event
Desktop->>Desktop: render ApplyPatchToolCallItem with diff UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/services/BuiltInToolService/service.ts (1)
436-441:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse nullish presence check when forwarding
displayResult.Line 440 uses a truthy check, so
''is silently dropped even when explicitly set. Use!= nullto preserve intentional empty display output.Suggested fix
options.emitToolEvent({ type: 'call_end', callId: options.toolCall.id, result: toolResult.result, - ...(toolResult.displayResult ? { displayResult: toolResult.displayResult } : {}), + ...(toolResult.displayResult != null + ? { displayResult: toolResult.displayResult } + : {}), isError: toolResult.isError, durationMs, finalStatus: toolResult.status === 'success' ? 'completed' : 'error', });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/services/BuiltInToolService/service.ts` around lines 436 - 441, The call to options.emitToolEvent is dropping intentionally empty display output because it uses a truthy check for toolResult.displayResult; update the spread that adds displayResult so it checks for presence with != null (e.g., toolResult.displayResult != null) instead of a truthy check, ensuring an explicit empty string is forwarded when emitting the 'call_end' event for options.toolCall.id while preserving existing structure and isError/result fields.apps/desktop/src/services/AgentService/session/history.ts (1)
268-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDerive status/error from the same restored result text.
resultnow usesrow.tool_output ?? row.content, but status/error mapping still usesrow.content. This can restore one text while computing status from another.Suggested fix
if (row.role === 'tool_result' && row.tool_call_id) { - const restoredStatus = mapPersistedToolResultStatus(row.tool_status, row.content); + const effectiveResultText = row.tool_output ?? row.content; + const restoredStatus = mapPersistedToolResultStatus( + row.tool_status, + effectiveResultText + ); currentEntry.toolResult = { callId: row.tool_call_id, - result: row.tool_output ?? row.content, + result: effectiveResultText, status: restoredStatus, durationMs: row.tool_duration_ms ?? undefined, - isError: isPersistedToolResultErrorStatus(row.tool_status, row.content), + isError: isPersistedToolResultErrorStatus( + row.tool_status, + effectiveResultText + ), }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/services/AgentService/session/history.ts` around lines 268 - 275, The status and error are derived from row.content while the stored result uses row.tool_output ?? row.content, causing a mismatch; compute a single restoredResult = row.tool_output ?? row.content and assign currentEntry.toolResult.result = restoredResult, then pass restoredResult into mapPersistedToolResultStatus(...) and isPersistedToolResultErrorStatus(...) (keep callId and durationMs assignments as-is) so both result and status/error are derived from the same text; update references in the block that builds currentEntry.toolResult accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src-tauri/src/core/built_in_tools/apply_patch.rs`:
- Around line 622-645: The write_staged_files function applies file
writes/deletes directly which can leave partial changes if an operation fails;
update it to perform an atomic-like commit by first staging writes to temporary
files (e.g., create temp sibling files for each PathBuf in staged_files), and
for deletes/mutates create backups of existing files before touching them; then
perform an ordered rename/move of temp files into place and remove backups only
after all moves succeed; on any error, roll back by deleting any newly created
temp files and restoring backups for files already overwritten or deleted.
Ensure you reference and update logic that iterates staged_files (variables path
and content) so that directory creation, fs::write, fs::remove_file are replaced
by temp-write + rename semantics and backup/restore handling to guarantee no
partial application remains on error.
- Around line 100-103: Move operations fail on non-UTF-8 files because
read_virtual_file (and the fs::read_to_string call inside it) is invoked even
when a patch is a rename-only (Move with no hunks); update the logic in
apply_patch.rs to detect a Move/rename with zero hunks and avoid calling
read_virtual_file/apply_hunks for that case—return or build the preview using
filenames/metadata (or short-circuit to treating content as unchanged) so we
never attempt to interpret file bytes as UTF-8 for rename-only patches; modify
the code paths around read_virtual_file, apply_hunks, and build_text_preview to
handle the rename-only case explicitly.
In `@apps/desktop/src/services/AgentService/task/projection/projection.ts`:
- Line 741: Replace the fallback from using the logical OR to nullish coalescing
for the tool call result: change the assignment of toolCall.result that
currently uses toolEvent.displayResult || toolEvent.result to use
toolEvent.displayResult ?? toolEvent.result so that empty strings are preserved
and only null/undefined trigger the fallback; update the code where
toolCall.result is set (referencing toolCall.result, toolEvent.displayResult,
toolEvent.result in projection.ts) accordingly.
In `@apps/desktop/src/services/BuiltInToolService/tools/applyPatch/constants.ts`:
- Around line 13-17: The runtime zod schema applyPatchArgsSchema currently marks
reason as optionalTrimmedStringSchema while the tool input requires it; update
the schema so reason is required (use nonEmptyTrimmedStringSchema or the
equivalent required trimmed string schema) in applyPatchArgsSchema and the other
occurrence referenced (around the second definition at line ~61) so runtime
parsing enforces a non-empty reason; locate the symbol applyPatchArgsSchema and
replace optionalTrimmedStringSchema for the reason field with the required
schema and run tests/type checks to ensure callers conform.
In `@apps/desktop/src/services/BuiltInToolService/tools/applyPatch/helper.ts`:
- Around line 51-74: The current parsing uses
raw.indexOf(APPLY_PATCH_RESULT_START/END) which can match substrings inside the
payload; update the boundary detection in the helper that computes
startIndex/endIndex so markers are matched only when they occur as full lines
(e.g., split raw by newline and find the line index where line.trim() ===
APPLY_PATCH_RESULT_START and similarly for APPLY_PATCH_RESULT_END), then compute
summary and payloadText using those line boundaries instead of substring index
hits (adjust how summary and payloadText are sliced after locating the marker
lines to preserve existing behavior for summary, workingDirectory, and
changedFiles extraction).
In `@apps/desktop/src/services/BuiltInToolService/tools/applyPatch/index.ts`:
- Around line 139-153: parseApplyPatchArgs is called outside the try/catch so
its validation errors bypass the tool's structured error response; move the
parseApplyPatchArgs call inside the existing try block (or wrap it in its own
try that rethrows into the same error handling path) so argument-parse failures
are caught and handled the same way as native.builtInTools.applyPatch errors,
preserving the existing error response shape that uses errorMessage, isError and
status; update references around parseApplyPatchArgs,
native.builtInTools.applyPatch and formatApplyPatchToolResult accordingly.
In `@apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts`:
- Around line 183-189: The current git checkout detection only matches "git
checkout -- <file>" using the regex against unquotedCommand and misses forms
like "git checkout HEAD -- file" or "git checkout <branch> -- file"; update the
checkout detection in
apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts (the block
that tests unquotedCommand) to use a broader pattern that detects any "git
checkout" invocation that contains "-- <path>" (for example, match "git
checkout" followed later by "-- <non-space>"), so the check returns {
isMutation: true, reason: 'Git working tree checkout' } for variants like "git
checkout HEAD -- file" and "git checkout <branch> -- file".
- Around line 168-174: The regexes testing unquotedCommand use dead anchors like
(^|\s) after a [\s\S]*?—replace those with a whitespace or end check and make
groups non-capturing: update the sed check to use something like
/\bsed\b[\s\S]*?\s-i(?:\s|$)/i and the perl check to use
/\bperl\b[\s\S]*?\s-p?i(?:\s|$)/i (operate on the same unquotedCommand variable
and return the same { isMutation, reason } responses).
In
`@apps/desktop/src/views/SearchView/components/ConversationPanel/components/BuiltInApplyPatchToolCallItem.vue`:
- Around line 83-84: Replace all hardcoded Chinese strings in
BuiltInApplyPatchToolCallItem.vue with i18n translations using the t() function:
change template text nodes (e.g., "二进制文件不显示改前改后内容", "变更预览", "已截断", "无内容",
"无内容变更", "工作目录未知") to use t('...') calls or interpolation ({{ t('key') }}) and
import/use the same i18n context as ToolCallItem.vue; update the status text
logic (the function returning hardcoded status strings around the
statusText/status mapping) to return translation keys like t('status.running'),
t('status.success'), t('status.failed'), t('status.unknown') (or semantically
named keys) instead of literal Chinese, and add corresponding keys for
binary/truncated/preview messages (e.g., binary_file_omitted, preview_changes,
truncated_note, no_content, no_content_changes, unknown_workdir) so the
component consistently uses t() for all displayed text.
In `@apps/desktop/src/views/SettingsView/components/BuiltInTools/types.ts`:
- Around line 89-91: Replace the hardcoded Chinese summary for toolId
'apply_patch' with the i18n call like the other tools (e.g. return
t('settings.builtInTools.summary.apply_patch')) in the BuiltInTools types logic,
and add the corresponding translation key
"settings.builtInTools.summary.apply_patch" to your locale files (provide
Chinese and English entries) so the summary follows the existing i18n pattern.
---
Outside diff comments:
In `@apps/desktop/src/services/AgentService/session/history.ts`:
- Around line 268-275: The status and error are derived from row.content while
the stored result uses row.tool_output ?? row.content, causing a mismatch;
compute a single restoredResult = row.tool_output ?? row.content and assign
currentEntry.toolResult.result = restoredResult, then pass restoredResult into
mapPersistedToolResultStatus(...) and isPersistedToolResultErrorStatus(...)
(keep callId and durationMs assignments as-is) so both result and status/error
are derived from the same text; update references in the block that builds
currentEntry.toolResult accordingly.
In `@apps/desktop/src/services/BuiltInToolService/service.ts`:
- Around line 436-441: The call to options.emitToolEvent is dropping
intentionally empty display output because it uses a truthy check for
toolResult.displayResult; update the spread that adds displayResult so it checks
for presence with != null (e.g., toolResult.displayResult != null) instead of a
truthy check, ensuring an explicit empty string is forwarded when emitting the
'call_end' event for options.toolCall.id while preserving existing structure and
isError/result fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d7693a8a-bef6-47fe-a8cc-6d8391853f11
📒 Files selected for processing (33)
apps/desktop/src-tauri/src/commands/built_in_tools.rsapps/desktop/src-tauri/src/commands/mod.rsapps/desktop/src-tauri/src/core/built_in_tools/apply_patch.rsapps/desktop/src-tauri/src/core/built_in_tools/mod.rsapps/desktop/src-tauri/src/core/built_in_tools/types.rsapps/desktop/src/database/artifacts/runtime/seed.sqlapps/desktop/src/database/queries/messages.tsapps/desktop/src/services/AgentService/contracts/tooling.tsapps/desktop/src/services/AgentService/prompt/builtin.tsapps/desktop/src/services/AgentService/session/history.tsapps/desktop/src/services/AgentService/task/projection/projection.tsapps/desktop/src/services/BuiltInToolService/registry.tsapps/desktop/src/services/BuiltInToolService/service.tsapps/desktop/src/services/BuiltInToolService/tools/applyPatch/constants.tsapps/desktop/src/services/BuiltInToolService/tools/applyPatch/helper.tsapps/desktop/src/services/BuiltInToolService/tools/applyPatch/index.tsapps/desktop/src/services/BuiltInToolService/tools/bash/constants.tsapps/desktop/src/services/BuiltInToolService/tools/bash/helper.tsapps/desktop/src/services/BuiltInToolService/tools/bash/index.tsapps/desktop/src/services/BuiltInToolService/types.tsapps/desktop/src/services/NativeService/builtInTools.tsapps/desktop/src/services/NativeService/index.tsapps/desktop/src/services/NativeService/types.tsapps/desktop/src/views/SearchView/components/ConversationPanel/components/BuiltInApplyPatchToolCallItem.vueapps/desktop/src/views/SearchView/components/ConversationPanel/components/ToolCallItem.vueapps/desktop/src/views/SettingsView/components/BuiltInTools/types.tsapps/desktop/tests/services/BuiltInToolService/tools/applyPatch/helper.test.tsapps/desktop/tests/services/BuiltInToolService/tools/applyPatch/index.test.tsapps/desktop/tests/services/BuiltInToolService/tools/bash/constants.test.tsapps/desktop/tests/services/BuiltInToolService/tools/bash/helper.test.tsapps/desktop/tests/services/BuiltInToolService/tools/bash/index.test.tsapps/desktop/tests/services/native-service.test.tsapps/desktop/tests/views/SearchView/components/ConversationPanel/components/BuiltInApplyPatchToolCallItem.test.ts
📜 Review details
🧰 Additional context used
🪛 SQLFluff (4.2.1)
apps/desktop/src/database/artifacts/runtime/seed.sql
[error] 108-108: The 'WHERE' keyword should always start a new line.
(LT14)
🔇 Additional comments (36)
apps/desktop/src/services/AgentService/contracts/tooling.ts (1)
125-125: LGTM!apps/desktop/src/services/BuiltInToolService/types.ts (1)
19-19: LGTM!Also applies to: 62-62
apps/desktop/src/database/artifacts/runtime/seed.sql (1)
104-113: LGTM!apps/desktop/src/database/queries/messages.ts (1)
17-17: LGTM!Also applies to: 30-30, 62-62, 214-214, 230-230
apps/desktop/src/services/AgentService/prompt/builtin.ts (1)
39-41: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/applyPatch/helper.test.ts (1)
1-47: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/applyPatch/index.test.ts (1)
1-148: LGTM!apps/desktop/tests/services/native-service.test.ts (1)
5-5: LGTM!Also applies to: 777-800, 818-834
apps/desktop/tests/views/SearchView/components/ConversationPanel/components/BuiltInApplyPatchToolCallItem.test.ts (1)
1-107: LGTM!apps/desktop/src-tauri/src/core/built_in_tools/types.rs (1)
7-57: LGTM!apps/desktop/src/services/NativeService/types.ts (1)
37-65: LGTM!apps/desktop/src-tauri/src/core/built_in_tools/apply_patch.rs (1)
293-389: LGTM!apps/desktop/src-tauri/src/core/built_in_tools/mod.rs (1)
5-5: LGTM!Also applies to: 12-12, 15-18
apps/desktop/src-tauri/src/commands/built_in_tools.rs (1)
6-8: LGTM!Also applies to: 11-17
apps/desktop/src-tauri/src/commands/mod.rs (1)
55-55: LGTM!apps/desktop/src/services/NativeService/builtInTools.ts (1)
3-8: LGTM!Also applies to: 14-18
apps/desktop/src/services/NativeService/index.ts (1)
30-34: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/bash/constants.test.ts (1)
37-40: LGTM!Also applies to: 71-77, 79-85, 169-188
apps/desktop/tests/services/BuiltInToolService/tools/bash/helper.test.ts (1)
7-7: LGTM!Also applies to: 57-69, 167-167, 185-185, 200-200, 260-265, 289-294, 301-342
apps/desktop/tests/services/BuiltInToolService/tools/bash/index.test.ts (1)
2-2: LGTM!Also applies to: 6-9, 156-189, 191-212
apps/desktop/src/services/BuiltInToolService/tools/bash/constants.ts (3)
36-41: LGTM!Also applies to: 64-69
110-123: LGTM!
128-139: LGTM!Also applies to: 167-175
apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts (2)
18-21: LGTM!Also applies to: 86-92
94-143: LGTM!apps/desktop/src/services/BuiltInToolService/tools/bash/index.ts (4)
37-41: LGTM!
124-127: Early-exit on mutation detection prevents user approval dialogs.When mutation is detected without
allowFileMutation, returningnullbypasses the approval dialog entirely—even whenapprovalMode === 'always'. The model then receives an error at execution time. This is intentional (no point showing approval for commands that will fail), but worth noting that user-configured "always approve" is overridden for blocked mutations.
166-183: LGTM!
258-263: LGTM!apps/desktop/src/views/SearchView/components/ConversationPanel/components/BuiltInApplyPatchToolCallItem.vue (1)
1-160: LGTM!Also applies to: 162-319, 325-378, 380-493, 495-639, 641-693, 695-1035
apps/desktop/src/views/SearchView/components/ConversationPanel/components/ToolCallItem.vue (1)
4-11: LGTM!Also applies to: 126-134
apps/desktop/src/views/SettingsView/components/BuiltInTools/types.ts (1)
76-84: LGTM!apps/desktop/src/services/BuiltInToolService/tools/applyPatch/constants.ts (1)
11-12: LGTM!Also applies to: 20-58
apps/desktop/src/services/BuiltInToolService/tools/applyPatch/helper.ts (1)
23-39: LGTM!Also applies to: 75-147
apps/desktop/src/services/BuiltInToolService/tools/applyPatch/index.ts (1)
24-129: LGTM!Also applies to: 163-190
apps/desktop/src/services/BuiltInToolService/registry.ts (1)
3-3: LGTM!Also applies to: 57-57
Comment on lines
+100
to
+103
| let current_content = read_virtual_file(&staged_files, &target)?; | ||
| let next_content = apply_hunks(¤t_content, &hunks, &target.display)?; | ||
| let preview = | ||
| build_text_preview(Some(current_content.as_str()), Some(next_content.as_str())); |
There was a problem hiding this comment.
Binary file moves are currently broken.
Move goes through read_virtual_file, and Line 448 uses fs::read_to_string, so non-UTF-8 files fail even when the patch is only renaming (*** Move to with no hunks). That contradicts the advertised Move support.
Also applies to: 441-450
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src-tauri/src/core/built_in_tools/apply_patch.rs` around lines
100 - 103, Move operations fail on non-UTF-8 files because read_virtual_file
(and the fs::read_to_string call inside it) is invoked even when a patch is a
rename-only (Move with no hunks); update the logic in apply_patch.rs to detect a
Move/rename with zero hunks and avoid calling read_virtual_file/apply_hunks for
that case—return or build the preview using filenames/metadata (or short-circuit
to treating content as unchanged) so we never attempt to interpret file bytes as
UTF-8 for rename-only patches; modify the code paths around read_virtual_file,
apply_hunks, and build_text_preview to handle the rename-only case explicitly.
Comment on lines
+622
to
+645
| fn write_staged_files(staged_files: &HashMap<PathBuf, Option<String>>) -> Result<(), String> { | ||
| for (path, content) in staged_files { | ||
| if let Some(content) = content { | ||
| if let Some(parent) = path.parent() { | ||
| fs::create_dir_all(parent).map_err(|error| { | ||
| format!( | ||
| "Failed to create parent directory for {:?}: {}", | ||
| path, error | ||
| ) | ||
| })?; | ||
| } | ||
| fs::write(path, content) | ||
| .map_err(|error| format!("Failed to write {:?}: {}", path, error))?; | ||
| } | ||
| } | ||
|
|
||
| for (path, content) in staged_files { | ||
| if content.is_none() && path.exists() { | ||
| fs::remove_file(path) | ||
| .map_err(|error| format!("Failed to delete {:?}: {}", path, error))?; | ||
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Staged filesystem commit is non-atomic and can leave partial mutations.
If any write/delete fails mid-loop, earlier file changes remain applied with no rollback. That can corrupt workspace state relative to the patch intent and tool response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src-tauri/src/core/built_in_tools/apply_patch.rs` around lines
622 - 645, The write_staged_files function applies file writes/deletes directly
which can leave partial changes if an operation fails; update it to perform an
atomic-like commit by first staging writes to temporary files (e.g., create temp
sibling files for each PathBuf in staged_files), and for deletes/mutates create
backups of existing files before touching them; then perform an ordered
rename/move of temp files into place and remove backups only after all moves
succeed; on any error, roll back by deleting any newly created temp files and
restoring backups for files already overwritten or deleted. Ensure you reference
and update logic that iterates staged_files (variables path and content) so that
directory creation, fs::write, fs::remove_file are replaced by temp-write +
rename semantics and backup/restore handling to guarantee no partial application
remains on error.
| toolEvent.callId, | ||
| (toolCall) => { | ||
| toolCall.result = toolEvent.result; | ||
| toolCall.result = toolEvent.displayResult || toolEvent.result; |
There was a problem hiding this comment.
Use nullish coalescing for display-result fallback.
|| treats empty string as absent and overwrites it with result. Use ?? to only fallback on null/undefined.
Suggested fix
- toolCall.result = toolEvent.displayResult || toolEvent.result;
+ toolCall.result = toolEvent.displayResult ?? toolEvent.result;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| toolCall.result = toolEvent.displayResult || toolEvent.result; | |
| toolCall.result = toolEvent.displayResult ?? toolEvent.result; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/services/AgentService/task/projection/projection.ts` at line
741, Replace the fallback from using the logical OR to nullish coalescing for
the tool call result: change the assignment of toolCall.result that currently
uses toolEvent.displayResult || toolEvent.result to use toolEvent.displayResult
?? toolEvent.result so that empty strings are preserved and only null/undefined
trigger the fallback; update the code where toolCall.result is set (referencing
toolCall.result, toolEvent.displayResult, toolEvent.result in projection.ts)
accordingly.
Comment on lines
+13
to
+17
| export const applyPatchArgsSchema = z.object({ | ||
| patch: nonEmptyTrimmedStringSchema, | ||
| workingDirectory: nonEmptyTrimmedStringSchema, | ||
| reason: optionalTrimmedStringSchema, | ||
| description: optionalTrimmedStringSchema, |
There was a problem hiding this comment.
Enforce reason in runtime schema too.
reason is required in the tool input schema, but optional in runtime parsing. That mismatch can still produce empty approval context when calls bypass model-schema generation.
Suggested fix
export const applyPatchArgsSchema = z.object({
patch: nonEmptyTrimmedStringSchema,
workingDirectory: nonEmptyTrimmedStringSchema,
- reason: optionalTrimmedStringSchema,
+ reason: nonEmptyTrimmedStringSchema,
description: optionalTrimmedStringSchema,
});Also applies to: 61-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/services/BuiltInToolService/tools/applyPatch/constants.ts`
around lines 13 - 17, The runtime zod schema applyPatchArgsSchema currently
marks reason as optionalTrimmedStringSchema while the tool input requires it;
update the schema so reason is required (use nonEmptyTrimmedStringSchema or the
equivalent required trimmed string schema) in applyPatchArgsSchema and the other
occurrence referenced (around the second definition at line ~61) so runtime
parsing enforces a non-empty reason; locate the symbol applyPatchArgsSchema and
replace optionalTrimmedStringSchema for the reason field with the required
schema and run tests/type checks to ensure callers conform.
Comment on lines
+51
to
+74
| const startIndex = raw.indexOf(APPLY_PATCH_RESULT_START); | ||
| if (startIndex < 0) { | ||
| return { | ||
| summary: raw, | ||
| workingDirectory: null, | ||
| changedFiles: [], | ||
| }; | ||
| } | ||
|
|
||
| const endIndex = raw.indexOf( | ||
| APPLY_PATCH_RESULT_END, | ||
| startIndex + APPLY_PATCH_RESULT_START.length | ||
| ); | ||
| if (endIndex < 0) { | ||
| return { | ||
| summary: raw, | ||
| workingDirectory: null, | ||
| changedFiles: [], | ||
| }; | ||
| } | ||
|
|
||
| const summary = raw.slice(0, startIndex).trim() || null; | ||
| const payloadText = raw.slice(startIndex + APPLY_PATCH_RESULT_START.length, endIndex).trim(); | ||
|
|
There was a problem hiding this comment.
Parse marker boundaries as full lines, not substring hits.
Current indexOf boundary detection can break if marker text appears inside serialized payload content. This causes avoidable parse fallback and lost structured preview data.
Suggested fix
- const startIndex = raw.indexOf(APPLY_PATCH_RESULT_START);
- if (startIndex < 0) {
+ const lines = raw.split('\n');
+ const startLineIndex = lines.findIndex((line) => line.trim() === APPLY_PATCH_RESULT_START);
+ if (startLineIndex < 0) {
return {
summary: raw,
workingDirectory: null,
changedFiles: [],
};
}
- const endIndex = raw.indexOf(
- APPLY_PATCH_RESULT_END,
- startIndex + APPLY_PATCH_RESULT_START.length
- );
- if (endIndex < 0) {
+ const endLineIndex = lines.findIndex(
+ (line, index) => index > startLineIndex && line.trim() === APPLY_PATCH_RESULT_END
+ );
+ if (endLineIndex < 0) {
return {
summary: raw,
workingDirectory: null,
changedFiles: [],
};
}
- const summary = raw.slice(0, startIndex).trim() || null;
- const payloadText = raw.slice(startIndex + APPLY_PATCH_RESULT_START.length, endIndex).trim();
+ const summary = lines.slice(0, startLineIndex).join('\n').trim() || null;
+ const payloadText = lines.slice(startLineIndex + 1, endLineIndex).join('\n').trim();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/services/BuiltInToolService/tools/applyPatch/helper.ts`
around lines 51 - 74, The current parsing uses
raw.indexOf(APPLY_PATCH_RESULT_START/END) which can match substrings inside the
payload; update the boundary detection in the helper that computes
startIndex/endIndex so markers are matched only when they occur as full lines
(e.g., split raw by newline and find the line index where line.trim() ===
APPLY_PATCH_RESULT_START and similarly for APPLY_PATCH_RESULT_END), then compute
summary and payloadText using those line boundaries instead of substring index
hits (adjust how summary and payloadText are sliced after locating the marker
lines to preserve existing behavior for summary, workingDirectory, and
changedFiles extraction).
Comment on lines
+139
to
+153
| const parsedArgs = parseApplyPatchArgs(args); | ||
| try { | ||
| const response = await native.builtInTools.applyPatch({ | ||
| patch: parsedArgs.patch, | ||
| workingDirectory: parsedArgs.workingDirectory, | ||
| }); | ||
|
|
||
| return { | ||
| result: response.summary, | ||
| displayResult: formatApplyPatchToolResult(response), | ||
| isError: false, | ||
| status: 'success', | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); |
There was a problem hiding this comment.
Keep argument-parse failures inside the structured error path.
parseApplyPatchArgs currently runs outside try/catch, so invalid arguments can escape the function instead of returning the standard tool error payload.
Suggested fix
- const parsedArgs = parseApplyPatchArgs(args);
try {
+ const parsedArgs = parseApplyPatchArgs(args);
const response = await native.builtInTools.applyPatch({
patch: parsedArgs.patch,
workingDirectory: parsedArgs.workingDirectory,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const parsedArgs = parseApplyPatchArgs(args); | |
| try { | |
| const response = await native.builtInTools.applyPatch({ | |
| patch: parsedArgs.patch, | |
| workingDirectory: parsedArgs.workingDirectory, | |
| }); | |
| return { | |
| result: response.summary, | |
| displayResult: formatApplyPatchToolResult(response), | |
| isError: false, | |
| status: 'success', | |
| }; | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| try { | |
| const parsedArgs = parseApplyPatchArgs(args); | |
| const response = await native.builtInTools.applyPatch({ | |
| patch: parsedArgs.patch, | |
| workingDirectory: parsedArgs.workingDirectory, | |
| }); | |
| return { | |
| result: response.summary, | |
| displayResult: formatApplyPatchToolResult(response), | |
| isError: false, | |
| status: 'success', | |
| }; | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : String(error); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/services/BuiltInToolService/tools/applyPatch/index.ts`
around lines 139 - 153, parseApplyPatchArgs is called outside the try/catch so
its validation errors bypass the tool's structured error response; move the
parseApplyPatchArgs call inside the existing try block (or wrap it in its own
try that rethrows into the same error handling path) so argument-parse failures
are caught and handled the same way as native.builtInTools.applyPatch errors,
preserving the existing error response shape that uses errorMessage, isError and
status; update references around parseApplyPatchArgs,
native.builtInTools.applyPatch and formatApplyPatchToolResult accordingly.
Comment on lines
+168
to
+174
| if (/\bsed\b[\s\S]*?(^|\s)-i(\s|$)/i.test(unquotedCommand)) { | ||
| return { isMutation: true, reason: 'in-place sed edit' }; | ||
| } | ||
|
|
||
| if (/\bperl\b[\s\S]*?(^|\s)-p?i(\s|$)/i.test(unquotedCommand)) { | ||
| return { isMutation: true, reason: 'in-place perl edit' }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Regex anchor ^ is ineffective mid-pattern.
In patterns like /\bsed\b[\s\S]*?(^|\s)-i(\s|$)/i, the ^ in (^|\s) can never match after [\s\S]*? has consumed characters since ^ only matches at position 0. The patterns work correctly because \s handles the intended case, but the ^ is dead code that may confuse future maintainers.
Consider simplifying to \s only:
♻️ Simplify regex patterns
- if (/\bsed\b[\s\S]*?(^|\s)-i(\s|$)/i.test(unquotedCommand)) {
+ if (/\bsed\b.*\s-i(\s|$)/i.test(unquotedCommand)) {
return { isMutation: true, reason: 'in-place sed edit' };
}
- if (/\bperl\b[\s\S]*?(^|\s)-p?i(\s|$)/i.test(unquotedCommand)) {
+ if (/\bperl\b.*\s-p?i(\s|$)/i.test(unquotedCommand)) {
return { isMutation: true, reason: 'in-place perl edit' };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts` around
lines 168 - 174, The regexes testing unquotedCommand use dead anchors like
(^|\s) after a [\s\S]*?—replace those with a whitespace or end check and make
groups non-capturing: update the sed check to use something like
/\bsed\b[\s\S]*?\s-i(?:\s|$)/i and the perl check to use
/\bperl\b[\s\S]*?\s-p?i(?:\s|$)/i (operate on the same unquotedCommand variable
and return the same { isMutation, reason } responses).
Comment on lines
+183
to
+189
| if (/\bgit\s+(mv|rm|restore|clean|reset)\b/i.test(unquotedCommand)) { | ||
| return { isMutation: true, reason: 'Git working tree mutation' }; | ||
| } | ||
|
|
||
| if (/\bgit\s+checkout\s+--\s+\S/i.test(unquotedCommand)) { | ||
| return { isMutation: true, reason: 'Git working tree checkout' }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Git checkout detection misses some forms.
The pattern /\bgit\s+checkout\s+--\s+\S/i only matches git checkout -- <file> but misses variants like git checkout HEAD -- file or git checkout <branch> -- file that also restore/overwrite working tree files.
Acceptable as a heuristic, but consider broadening if coverage matters:
♻️ Broaden git checkout detection
- if (/\bgit\s+checkout\s+--\s+\S/i.test(unquotedCommand)) {
+ if (/\bgit\s+checkout\b.*\s--\s+\S/i.test(unquotedCommand)) {
return { isMutation: true, reason: 'Git working tree checkout' };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (/\bgit\s+(mv|rm|restore|clean|reset)\b/i.test(unquotedCommand)) { | |
| return { isMutation: true, reason: 'Git working tree mutation' }; | |
| } | |
| if (/\bgit\s+checkout\s+--\s+\S/i.test(unquotedCommand)) { | |
| return { isMutation: true, reason: 'Git working tree checkout' }; | |
| } | |
| if (/\bgit\s+(mv|rm|restore|clean|reset)\b/i.test(unquotedCommand)) { | |
| return { isMutation: true, reason: 'Git working tree mutation' }; | |
| } | |
| if (/\bgit\s+checkout\b.*\s--\s+\S/i.test(unquotedCommand)) { | |
| return { isMutation: true, reason: 'Git working tree checkout' }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts` around
lines 183 - 189, The current git checkout detection only matches "git checkout
-- <file>" using the regex against unquotedCommand and misses forms like "git
checkout HEAD -- file" or "git checkout <branch> -- file"; update the checkout
detection in apps/desktop/src/services/BuiltInToolService/tools/bash/helper.ts
(the block that tests unquotedCommand) to use a broader pattern that detects any
"git checkout" invocation that contains "-- <path>" (for example, match "git
checkout" followed later by "-- <non-space>"), so the check returns {
isMutation: true, reason: 'Git working tree checkout' } for variants like "git
checkout HEAD -- file" and "git checkout <branch> -- file".
Comment on lines
+83
to
+84
| 二进制文件不显示改前改后内容 | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Hardcoded Chinese strings should use i18n.
The component has multiple hardcoded Chinese strings that should use the t() translation function for consistency with the rest of the codebase:
- Lines 83-84, 88-90: Binary/omitted file messages
- Line 93: "变更预览" header
- Lines 100-102: "已截断" truncation note
- Lines 250-254, 256: "无内容" / "无内容变更" / "工作目录未知"
- Lines 260-261, 264: Status fallback texts
- Lines 298-299, 648-668: Status text function returns hardcoded Chinese
ToolCallItem.vue uses t() for similar status text (e.g., t('common.running')), while this component returns hardcoded '运行中'.
Also applies to: 88-90, 93-93, 100-102, 250-254, 256-256, 260-261, 264-264, 648-668, 298-299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/views/SearchView/components/ConversationPanel/components/BuiltInApplyPatchToolCallItem.vue`
around lines 83 - 84, Replace all hardcoded Chinese strings in
BuiltInApplyPatchToolCallItem.vue with i18n translations using the t() function:
change template text nodes (e.g., "二进制文件不显示改前改后内容", "变更预览", "已截断", "无内容",
"无内容变更", "工作目录未知") to use t('...') calls or interpolation ({{ t('key') }}) and
import/use the same i18n context as ToolCallItem.vue; update the status text
logic (the function returning hardcoded status strings around the
statusText/status mapping) to return translation keys like t('status.running'),
t('status.success'), t('status.failed'), t('status.unknown') (or semantically
named keys) instead of literal Chinese, and add corresponding keys for
binary/truncated/preview messages (e.g., binary_file_omitted, preview_changes,
truncated_note, no_content, no_content_changes, unknown_workdir) so the
component consistently uses t() for all displayed text.
Comment on lines
+89
to
+91
| if (toolId === 'apply_patch') { | ||
| return '使用补丁语法修改本地文件'; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Summary should use i18n like other tools.
The apply_patch summary returns a hardcoded Chinese string while all other tools use t('settings.builtInTools.summary.xxx'). This breaks the i18n pattern and will not translate for non-Chinese users.
if (toolId === 'apply_patch') {
- return '使用补丁语法修改本地文件';
+ return t('settings.builtInTools.summary.applyPatch');
}You'll need to add the corresponding translation key to the i18n files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (toolId === 'apply_patch') { | |
| return '使用补丁语法修改本地文件'; | |
| } | |
| if (toolId === 'apply_patch') { | |
| return t('settings.builtInTools.summary.applyPatch'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/views/SettingsView/components/BuiltInTools/types.ts` around
lines 89 - 91, Replace the hardcoded Chinese summary for toolId 'apply_patch'
with the i18n call like the other tools (e.g. return
t('settings.builtInTools.summary.apply_patch')) in the BuiltInTools types logic,
and add the corresponding translation key
"settings.builtInTools.summary.apply_patch" to your locale files (provide
Chinese and English entries) so the summary follows the existing i18n pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #127 by adding a dedicated
apply_patchbuilt-in tool for structured local file edits.This gives TouchAI a safer and more reviewable editing primitive than shell-based file mutation for routine source changes.
Related issue or RFC
Related to #127
AI assistance disclosure
This PR was developed with the assistance of an AI coding tool for code generation and logic suggestions.
Testing evidence
applyPatchare passingBuiltInApplyPatchToolCallItemUI tests are passingtauri devwas able to compile and launch locally after dependency cache/network retryRisk notes
workingDirectoryScreenshots or recordings
Checklist
apply_patchbuilt-in tool path