fix(vscode): prevent edit_existing_file from silently failing without error feedback#12756
Open
rodboev wants to merge 3 commits into
Open
fix(vscode): prevent edit_existing_file from silently failing without error feedback#12756rodboev wants to merge 3 commits into
rodboev wants to merge 3 commits into
Conversation
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
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
Fixes #12317
When using the
edit_existing_filetool in VS Code, edits can silently fail: the chat reports "File edited successfully" but the file remains unchanged. This happens because the apply-state lifecycle can stall without reaching the "closed" status, and because non-ASCII file paths can fail URI resolution.Root cause
Two interacting failure modes:
Apply state lifecycle stall: The
edit_existing_filetool dispatches an apply operation and waits for VS Code'sVerticalDiffManagerto complete the diff streaming and emit a "closed" status. IfdeterministicApplyLazyEditfails (common for insertions where AST matching can't find corresponding nodes), the fallbackstreamLazyApplyruns an LLM-based diff. If that stream produces no output, errors, or is aborted,streamDiffLines()can return without emitting "closed". The tool call is stuck in "calling" status indefinitely, and the LLM's pre-generated "File edited successfully" text appears as a false success signal.Non-ASCII path resolution failure:
editToolImplcallsresolveRelativePathInDir()which uses URI-encoded paths withfileExists(). Non-ASCII characters in file paths cause the URI encoding to mismatch the filesystem, makingfileExistsreturn false. The tool throws a "does not exist" error, but this error can be swallowed in certain UI states.Changes
streamDiffLines()andinstantApplyDiff()inVerticalDiffManagerwith try/finally blocks that guarantee a "closed" status update is always emitted, even if the diff stream yields nothing or throwsapplyForEditToolthat detects apply states stuck without reaching "closed" or "done" and dispatches an error after 60 seconds (excludes "done" state so user review time doesn't trigger false timeout)doneStreamsset to distinguish successful edits (which transition through "done" before "closed") from bail-out failures (which go directly to "closed"), preventing false "Edit Success" reports for edits that never generated diffseditToolImplfor non-ASCII filenames, attempting directfileExists()with the original path before falling back to the open-files checkWhat this doesn't change
deterministicApplyLazyEdit,streamLazyApply, orapplyCodeBlockis untouched; this fix addresses the error reporting and lifecycle management, not the diff algorithm itselfsingle_find_and_replaceandmulti_edittools share the sameapplyForEditTooldispatch and benefit from the timeout fix without additional changes<tool_call>raw output reported by AndyP2) is a separate issue related to tool call parsing in chat mode, not addressed hereChecklist
Tests
gui/src/redux/thunks/handleApplyStateUpdate.test.ts- updated/new tests for timeout safety net behavioredit_existing_filewith insertion-only changes, verify either the edit applies or a clear error message appears in the chatedit_existing_file, verify it either edits correctly or reports a clear path resolution errorSummary by cubic
Stops
edit_existing_filein VS Code from reporting success when no changes were applied by fixing the apply lifecycle and surfacing errors. Also adds a timeout for stalled applies and fixes non-ASCII path resolution. Fixes #12317.VerticalDiffManagerusing try/finally; send an error-state update when handler creation fails.applyForEditToolto flag stalled applies; clear the timer reliably and ignore normal "done" waits.editToolImpl.single_find_and_replaceandmulti_editsince they share the same apply flow.Written for commit 942d69f. Summary will update on new commits.