diff --git a/extensions/vscode/src/diff/vertical/manager.ts b/extensions/vscode/src/diff/vertical/manager.ts index 059efe0aee4..b0b32bc464a 100644 --- a/extensions/vscode/src/diff/vertical/manager.ts +++ b/extensions/vscode/src/diff/vertical/manager.ts @@ -216,6 +216,13 @@ export class VerticalDiffManager { // Get the current editor fileUri/range let editor = vscode.window.activeTextEditor; if (!editor) { + // Emit closed status so the tool call doesn't hang + void this.webviewProtocol.request("updateApplyState", { + streamId, + status: "closed", + numDiffs: 0, + toolCallId, + }); return; } const fileUri = editor.document.uri.toString(); @@ -254,6 +261,15 @@ export class VerticalDiffManager { if (!diffHandler) { console.warn("Issue occurred while creating new vertical diff handler"); + // Emit closed status so the tool call doesn't hang + void this.webviewProtocol.request("updateApplyState", { + streamId, + status: "closed", + numDiffs: 0, + fileContent: editor.document.getText(), + filepath: fileUri, + toolCallId, + }); return; } @@ -305,6 +321,13 @@ export class VerticalDiffManager { const editor = vscode.window.activeTextEditor; if (!editor) { + // Emit closed status so the tool call doesn't hang + void this.webviewProtocol.request("updateApplyState", { + streamId, + status: "closed", + numDiffs: 0, + toolCallId, + }); return; } @@ -333,6 +356,15 @@ export class VerticalDiffManager { if (!diffHandler) { console.warn("Issue occurred while creating vertical diff handler"); + // Emit closed status so the tool call doesn't hang + void this.webviewProtocol.request("updateApplyState", { + streamId, + status: "closed", + numDiffs: 0, + fileContent: editor.document.getText(), + filepath: fileUri, + toolCallId, + }); return; } diff --git a/gui/src/redux/thunks/handleApplyStateUpdate.test.ts b/gui/src/redux/thunks/handleApplyStateUpdate.test.ts index 2e426b7d7c1..4a086cc0318 100644 --- a/gui/src/redux/thunks/handleApplyStateUpdate.test.ts +++ b/gui/src/redux/thunks/handleApplyStateUpdate.test.ts @@ -170,7 +170,7 @@ describe("handleApplyStateUpdate", () => { status: "done", ...UNUSED_TOOL_CALL_PARAMS, }; - const newApplyState = { streamId: "chat-stream" }; + const newApplyState = { streamId: "chat-stream", numDiffs: 1 }; vi.mocked(findToolCallById).mockReturnValue(toolCallState); mockGetState.mockReturnValue({ @@ -181,14 +181,26 @@ describe("handleApplyStateUpdate", () => { }, }, config: { config: {} }, + ui: { toolSettings: {} }, }); + // Simulate the "done" event that precedes "closed" in normal flow + const doneState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "done", + filepath: "test.txt", + numDiffs: 1, + }; + const doneThunk = handleApplyStateUpdate(doneState); + await doneThunk(mockDispatch, mockGetState, mockExtra); + const applyState: ApplyState = { streamId: "chat-stream", toolCallId: "test-tool-call", status: "closed", filepath: "test.txt", - numDiffs: 1, + numDiffs: 0, }; const thunk = handleApplyStateUpdate(applyState); @@ -222,7 +234,7 @@ describe("handleApplyStateUpdate", () => { status: "canceled", ...UNUSED_TOOL_CALL_PARAMS, }; - const newApplyState = { streamId: "chat-stream" }; + const newApplyState = { streamId: "chat-stream-cancel" }; vi.mocked(findToolCallById).mockReturnValue(toolCallState); mockGetState.mockReturnValue({ @@ -236,11 +248,11 @@ describe("handleApplyStateUpdate", () => { }); const applyState: ApplyState = { - streamId: "chat-stream", + streamId: "chat-stream-cancel", toolCallId: "test-tool-call", status: "closed", filepath: "test.txt", - numDiffs: 1, + numDiffs: 0, }; const thunk = handleApplyStateUpdate(applyState); @@ -270,7 +282,7 @@ describe("handleApplyStateUpdate", () => { status: "errored", ...UNUSED_TOOL_CALL_PARAMS, }; - const newApplyState = { streamId: "chat-stream" }; + const newApplyState = { streamId: "chat-stream-error" }; vi.mocked(findToolCallById).mockReturnValue(toolCallState); mockGetState.mockReturnValue({ @@ -284,17 +296,66 @@ describe("handleApplyStateUpdate", () => { }); const applyState: ApplyState = { - streamId: "chat-stream", + streamId: "chat-stream-error", toolCallId: "test-tool-call", status: "closed", filepath: "test.txt", - numDiffs: 1, + numDiffs: 0, + }; + + const thunk = handleApplyStateUpdate(applyState); + await thunk(mockDispatch, mockGetState, mockExtra); + + expect(acceptToolCall).not.toHaveBeenCalled(); + expect(streamResponseAfterToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + }); + + it("should reject closure that skipped done (bail-out path)", async () => { + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const newApplyState = { streamId: "bail-stream" }; + + vi.mocked(findToolCallById).mockReturnValue(toolCallState); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { + states: [newApplyState], + }, + }, + config: { config: {} }, + }); + + // No "done" event dispatched — simulates handler bail-out + const applyState: ApplyState = { + streamId: "bail-stream", + toolCallId: "test-tool-call", + status: "closed", + filepath: "test.txt", + numDiffs: 0, }; const thunk = handleApplyStateUpdate(applyState); await thunk(mockDispatch, mockGetState, mockExtra); expect(acceptToolCall).not.toHaveBeenCalled(); + expect(updateToolCallOutput).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + contextItems: [ + { + name: "Edit Failed", + content: + "Failed to edit test.txt. To continue working with the file, read it again to see the most up-to-date contents", + description: "", + hidden: true, + }, + ], + }); expect(streamResponseAfterToolCall).toHaveBeenCalledWith({ toolCallId: "test-tool-call", }); @@ -343,14 +404,26 @@ describe("handleApplyStateUpdate", () => { }, }, config: { config: {} }, + ui: { toolSettings: {} }, }); + // Simulate the "done" event + const doneState: ApplyState = { + streamId: "chat-stream", + toolCallId: "test-tool-call", + status: "done", + filepath: "test.txt", + numDiffs: 1, + }; + const doneThunk = handleApplyStateUpdate(doneState); + await doneThunk(mockDispatch, mockGetState, mockExtra); + const applyState: ApplyState = { streamId: "chat-stream", toolCallId: "test-tool-call", status: "closed", filepath: "test.txt", - numDiffs: 1, + numDiffs: 0, }; const thunk = handleApplyStateUpdate(applyState); @@ -382,7 +455,6 @@ describe("handleApplyStateUpdate", () => { it("should handle different status values", async () => { const statusValues: ApplyState["status"][] = [ - "not-started", "streaming", "done", "closed", @@ -391,8 +463,17 @@ describe("handleApplyStateUpdate", () => { for (const status of statusValues) { vi.clearAllMocks(); + vi.mocked(findToolCallById).mockReturnValue(undefined); + mockGetState.mockReturnValue({ + session: { + history: [], + codeBlockApplyStates: { states: [] }, + }, + ui: { toolSettings: {} }, + }); + const applyState: ApplyState = { - streamId: "chat-stream", + streamId: `chat-stream-${status}`, toolCallId: "test-tool-call", status, filepath: "test.txt", @@ -666,6 +747,179 @@ describe("applyForEditTool", () => { }); }); + describe("timeout handling", () => { + it("should dispatch error on apply timeout", async () => { + vi.useFakeTimers(); + + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "not-started", + streamId: "test-stream", + toolCallId: "test-tool-call", + }; + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + // Simulate the apply request never completing (never calling a callback) + let resolveApplyRequest: (value: any) => void; + const applyPromise = new Promise((resolve) => { + resolveApplyRequest = resolve; + }); + mockExtra.ideMessenger.request.mockReturnValue(applyPromise); + + const thunk = applyForEditTool(payload); + const thunkPromise = thunk(mockDispatch, mockGetState, mockExtra); + + // Advance time past the timeout (60 seconds) + vi.advanceTimersByTime(61_000); + + // Allow promises to settle + await vi.runAllTimersAsync(); + + expect(errorToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + expect(updateToolCallOutput).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + contextItems: [ + { + icon: "problems", + name: "Apply Timeout", + description: "Edit operation timed out", + content: + "Error editing file: the apply operation did not complete within the expected time. The file may not have been modified. Please try again or use a different approach.\n\nPlease try something else or request further instructions.", + hidden: false, + }, + ], + }); + + // Verify that a dispatch with type 'apply/updateState' was made (for handleApplyStateUpdate) + expect(mockDispatch).toHaveBeenCalledWith( + expect.objectContaining({ + type: expect.stringContaining("apply"), + }), + ); + + vi.useRealTimers(); + }); + + it("should not trigger timeout when apply completes normally", async () => { + vi.useFakeTimers(); + vi.clearAllMocks(); + + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call-normal", + streamId: "test-stream-normal", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call-normal", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "not-started", + streamId: "test-stream-normal", + toolCallId: "test-tool-call-normal", + }; + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + // Simulate successful apply request + mockExtra.ideMessenger.request.mockResolvedValue({ status: "success" }); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + // Simulate the apply state reaching "closed" status via the normal flow + // This would happen when VS Code sends back the status update + const closedApplyState: ApplyState = { + status: "closed", + streamId: "test-stream-normal", + toolCallId: "test-tool-call-normal", + }; + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(closedApplyState); + + // Advance time past the timeout + vi.advanceTimersByTime(61_000); + + // The timeout should not have dispatched error because the apply state reached "closed" + expect(errorToolCall).not.toHaveBeenCalled(); + + vi.useRealTimers(); + }); + + it("should clear timeout on immediate error", async () => { + vi.useFakeTimers(); + + const payload: ApplyToFilePayload & { toolCallId: string } = { + toolCallId: "test-tool-call", + streamId: "test-stream", + filepath: "test.txt", + text: "new content", + isSearchAndReplace: true, + }; + + const toolCallState: ToolCallState = { + toolCallId: "test-tool-call", + status: "calling", + ...UNUSED_TOOL_CALL_PARAMS, + }; + const applyState: ApplyState = { + status: "not-started", + streamId: "test-stream", + toolCallId: "test-tool-call", + }; + + vi.mocked(selectToolCallById).mockReturnValue(toolCallState); + vi.mocked(selectApplyStateByToolCallId).mockReturnValue(applyState); + mockGetState.mockReturnValue({}); + + mockExtra.ideMessenger.request.mockRejectedValue( + new Error("Request failed"), + ); + + const thunk = applyForEditTool(payload); + await thunk(mockDispatch, mockGetState, mockExtra); + + // Check that error was dispatched (from immediate error, not timeout) + expect(errorToolCall).toHaveBeenCalledWith({ + toolCallId: "test-tool-call", + }); + + const callCountAfterError = vi.mocked(errorToolCall).mock.calls.length; + + // Advance time past the timeout + vi.advanceTimersByTime(61_000); + + // Error should only have been dispatched once (from the immediate error) + expect(vi.mocked(errorToolCall).mock.calls.length).toBe( + callCountAfterError, + ); + + vi.useRealTimers(); + }); + }); + describe("edge cases", () => { it("should handle different payload types", async () => { const payloads: (ApplyToFilePayload & { toolCallId: string })[] = [ diff --git a/gui/src/redux/thunks/handleApplyStateUpdate.ts b/gui/src/redux/thunks/handleApplyStateUpdate.ts index a220af51f63..f980a8be611 100644 --- a/gui/src/redux/thunks/handleApplyStateUpdate.ts +++ b/gui/src/redux/thunks/handleApplyStateUpdate.ts @@ -18,6 +18,11 @@ import { findToolCallById, logToolUsage } from "../util"; import { exitEdit } from "./edit"; import { streamResponseAfterToolCall } from "./streamResponseAfterToolCall"; +// Tracks streams that reached "done" (diffs generated and awaiting user action). +// Consumed when "closed" fires to distinguish successful edits from bail-outs +// where the handler was never created. +const doneStreams = new Set(); + export const handleApplyStateUpdate = createAsyncThunk< void, ApplyState, @@ -61,17 +66,23 @@ export const handleApplyStateUpdate = createAsyncThunk< }); } + if (applyState.status === "done") { + doneStreams.add(applyState.streamId); + } + if (applyState.status === "closed") { + const reachedDone = doneStreams.delete(applyState.streamId); if (toolCallState) { - const accepted = toolCallState.status !== "canceled"; - - logToolUsage(toolCallState, accepted, true, extra.ideMessenger); - - // Log edit outcome for Agent Mode const newApplyState = getState().session.codeBlockApplyStates.states.find( (s) => s.streamId === applyState.streamId, ); + const accepted = + toolCallState.status !== "canceled" && + toolCallState.status !== "errored" && + reachedDone; + + logToolUsage(toolCallState, accepted, true, extra.ideMessenger); const newState = getState(); if (newApplyState) { void logAgentModeEditOutcome( @@ -85,52 +96,36 @@ export const handleApplyStateUpdate = createAsyncThunk< } if (accepted) { - if (toolCallState.status !== "errored") { + dispatch( + acceptToolCall({ + toolCallId: applyState.toolCallId, + }), + ); + + // Add autoformatting diff to tool output if present + if (applyState.autoFormattingDiff) { dispatch( - acceptToolCall({ + updateToolCallOutput({ toolCallId: applyState.toolCallId, + contextItems: [ + { + icon: "info", + name: "Auto-formatting Applied", + description: "Editor auto-formatting changes", + content: `Along with your edits, the editor applied the following auto-formatting:\n\n${applyState.autoFormattingDiff}\n\n(Note: Pay close attention to changes such as single quotes being converted to double quotes, semicolons being removed or added, long lines being broken into multiple lines, adjusting indentation style, adding/removing trailing commas, etc. This will help you ensure future SEARCH/REPLACE operations to this file are accurate.)`, + hidden: false, + }, + ], }), ); - - // Add autoformatting diff to tool output if present - if (applyState.autoFormattingDiff) { - dispatch( - updateToolCallOutput({ - toolCallId: applyState.toolCallId, - contextItems: [ - { - icon: "info", - name: "Auto-formatting Applied", - description: "Editor auto-formatting changes", - content: `Along with your edits, the editor applied the following auto-formatting:\n\n${applyState.autoFormattingDiff}\n\n(Note: Pay close attention to changes such as single quotes being converted to double quotes, semicolons being removed or added, long lines being broken into multiple lines, adjusting indentation style, adding/removing trailing commas, etc. This will help you ensure future SEARCH/REPLACE operations to this file are accurate.)`, - hidden: false, - }, - ], - }), - ); - } else { - dispatch( - updateToolCallOutput({ - toolCallId: applyState.toolCallId, - contextItems: [ - { - name: "Edit Success", - content: `Successfully edited ${applyState.filepath}`, - description: "", - hidden: true, - }, - ], - }), - ); - } } else { dispatch( updateToolCallOutput({ toolCallId: applyState.toolCallId, contextItems: [ { - name: "Edit Failed", - content: `Failed to edit ${applyState.filepath}. To continue working with the file, read it again to see the most up-to-date contents`, + name: "Edit Success", + content: `Successfully edited ${applyState.filepath}`, description: "", hidden: true, }, @@ -138,7 +133,25 @@ export const handleApplyStateUpdate = createAsyncThunk< }), ); } - + void dispatch( + streamResponseAfterToolCall({ + toolCallId: applyState.toolCallId, + }), + ); + } else if (toolCallState.status !== "canceled") { + dispatch( + updateToolCallOutput({ + toolCallId: applyState.toolCallId, + contextItems: [ + { + name: "Edit Failed", + content: `Failed to edit ${applyState.filepath}. To continue working with the file, read it again to see the most up-to-date contents`, + description: "", + hidden: true, + }, + ], + }), + ); void dispatch( streamResponseAfterToolCall({ toolCallId: applyState.toolCallId, @@ -166,6 +179,42 @@ export const applyForEditTool = createAsyncThunk< }), ); + // Set up a timeout to detect stalled apply operations + const APPLY_TIMEOUT_MS = 60_000; + const timeoutId = setTimeout(() => { + const state = getState(); + const applyState = selectApplyStateByToolCallId(state, toolCallId); + const toolCallState = selectToolCallById(state, toolCallId); + + if ( + applyState && + applyState.status !== "closed" && + applyState.status !== "done" && + toolCallState?.status === "calling" + ) { + dispatch( + errorToolCall({ + toolCallId, + }), + ); + dispatch( + updateToolCallOutput({ + toolCallId, + contextItems: [ + { + icon: "problems", + name: "Apply Timeout", + description: "Edit operation timed out", + content: + "Error editing file: the apply operation did not complete within the expected time. The file may not have been modified. Please try again or use a different approach.\n\nPlease try something else or request further instructions.", + hidden: false, + }, + ], + }), + ); + } + }, APPLY_TIMEOUT_MS); + let didError = false; try { const response = await extra.ideMessenger.request("applyToFile", payload); @@ -175,6 +224,7 @@ export const applyForEditTool = createAsyncThunk< } catch (e) { didError = true; } + clearTimeout(timeoutId); if (didError) { const state = getState(); diff --git a/gui/src/util/clientTools/editImpl.ts b/gui/src/util/clientTools/editImpl.ts index d26826981ec..cc74c6f2906 100644 --- a/gui/src/util/clientTools/editImpl.ts +++ b/gui/src/util/clientTools/editImpl.ts @@ -23,6 +23,15 @@ export const editToolImpl: ClientToolImpl = async ( extras.ideMessenger.ide, ); + // Fallback: try the raw filepath directly (handles non-ASCII paths + // where URI encoding causes resolveRelativePathInDir to fail) + if (!firstUriMatch) { + const exists = await extras.ideMessenger.ide.fileExists(filepath); + if (exists) { + firstUriMatch = filepath; + } + } + if (!firstUriMatch) { const openFiles = await extras.ideMessenger.ide.getOpenFiles(); for (const uri of openFiles) {