From 229cdbdd5d7f40cec86a07f929e10eb8a833d6fa Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Mon, 24 Aug 2026 15:40:09 -0500 Subject: [PATCH] fix(ai-core): Save the file edit_file actually edited EditorBufferApplier persisted its edit by focusing the target tab and calling saveCurrentFile(). Both are fire-and-forget: openFile() only posts the tab switch, so saveCurrentFile() read the previous tab index and saved whatever the user was looking at - force-saving their in-progress work while the target file stayed dirty and unchanged on disk. It returned true either way, so the model was told "saved" and later builds compiled the stale copy. Use the host's new file-targeted IdeEditorService.saveFile(File) instead. The buffer read and the replaceRange still share one main-thread block; the save moves to ioDispatcher, because saveFile blocks until the bytes are on disk and the write itself runs on main. openFile is gone from this path entirely - the agent no longer takes the user's focus to persist a file. saveFile throws on a permission miss rather than returning false, so the call is wrapped: an exception escaping here would abort the whole agent turn over an edit that already landed in the buffer. Swallowing it into saved=false yields "applied, left unsaved", which is true and actionable. New EditorBufferApplierTest covers the unfocused-file case with negative verifications on openFile/saveCurrentFile - the lock against reintroducing the focus dependency - plus save success, failure, throwing, and the two refusal paths. EditFileHandlerTest moves off the removed stubs. Requires the host release that ships saveFile (26.35); min_ide_version bumped, though the host records rather than enforces it. --- ai-core/src/main/AndroidManifest.xml | 2 +- .../tool/handlers/edit/EditorBufferApplier.kt | 27 +-- .../tool/handlers/EditFileHandlerTest.kt | 32 ++-- .../handlers/edit/EditorBufferApplierTest.kt | 155 ++++++++++++++++++ 4 files changed, 189 insertions(+), 27 deletions(-) create mode 100644 ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/EditorBufferApplierTest.kt diff --git a/ai-core/src/main/AndroidManifest.xml b/ai-core/src/main/AndroidManifest.xml index 1f426b6c..b9756ed9 100644 --- a/ai-core/src/main/AndroidManifest.xml +++ b/ai-core/src/main/AndroidManifest.xml @@ -28,7 +28,7 @@ + android:value="26.35" /> () val replacement = slot() @@ -487,7 +489,10 @@ class EditFileHandlerTest { assertEquals(1, range.captured.endLine) assertEquals(5, range.captured.endColumn) assertEquals("LINE1", replacement.captured) - verify { editorService.saveCurrentFile() } + coVerify { editorService.saveFile(target) } + // The save is file-targeted: the user's tab focus must not be read or moved (ADFA-5215). + verify(exactly = 0) { editorService.openFile(any()) } + verify(exactly = 0) { editorService.saveCurrentFile() } } @Test @@ -496,7 +501,7 @@ class EditFileHandlerTest { val file = createFile("Main.kt", "val a = 1\n") every { editorService.getFileContent(file) } returns "val a = 1\nval userTyped = 2\n" every { editorService.replaceRange(any(), any(), any()) } returns true - every { editorService.saveCurrentFile() } returns true + coEvery { editorService.saveFile(any()) } returns true val result = edit( "file_path" to "Main.kt", @@ -518,7 +523,7 @@ class EditFileHandlerTest { val file = createFile("Main.kt", "a\na\n") every { editorService.getFileContent(file) } returns "a\na\n" every { editorService.replaceRange(any(), any(), any()) } returns true - every { editorService.saveCurrentFile() } returns true + coEvery { editorService.saveFile(any()) } returns true val range = slot() val replacement = slot() @@ -538,12 +543,11 @@ class EditFileHandlerTest { } @Test - fun givenTheTabCannotBeFocused_whenEdited_thenNothingIsSavedAndTheResultSaysUnsaved() { - // saveCurrentFile() saves the FOCUSED tab, so saving unfocused persists a different file. + fun givenTheHostCannotSaveTheFile_whenEdited_thenTheResultSaysUnsaved() { val file = createFile("Main.kt", "old\n") every { editorService.getFileContent(file) } returns "old\n" every { editorService.replaceRange(any(), any(), any()) } returns true - every { editorService.openFile(any()) } returns false + coEvery { editorService.saveFile(any()) } returns false val result = edit("file_path" to "Main.kt", "old_string" to "old", "new_string" to "new") @@ -552,6 +556,8 @@ class EditFileHandlerTest { "Must not claim the file was saved; got: ${result.message}", result.message.contains("left unsaved") ) + // A failed save must not be retried by focusing the tab and saving that instead. + verify(exactly = 0) { editorService.openFile(any()) } verify(exactly = 0) { editorService.saveCurrentFile() } assertEquals("old\n", file.readText()) } @@ -566,7 +572,7 @@ class EditFileHandlerTest { assertFalse(result.success) assertEquals("old\n", file.readText()) - verify(exactly = 0) { editorService.saveCurrentFile() } + coVerify(exactly = 0) { editorService.saveFile(any()) } } @Test @@ -589,7 +595,7 @@ class EditFileHandlerTest { result.message.contains("changed") ) verify(exactly = 0) { editorService.replaceRange(any(), any(), any()) } - verify(exactly = 0) { editorService.saveCurrentFile() } + coVerify(exactly = 0) { editorService.saveFile(any()) } assertEquals("the disk copy must not be touched either", analysed, file.readText()) } @@ -602,7 +608,7 @@ class EditFileHandlerTest { "line0\nline1\n", ) every { editorService.replaceRange(any(), any(), any()) } returns true - every { editorService.saveCurrentFile() } returns true + coEvery { editorService.saveFile(any()) } returns true val result = edit("file_path" to "Main.kt", "old_string" to "line1", "new_string" to "LINE1") @@ -680,7 +686,7 @@ class EditFileHandlerTest { assertEquals("val a = 1\n", file.readText()) verify(exactly = 0) { editorService.replaceRange(any(), any(), any()) } - verify(exactly = 0) { editorService.saveCurrentFile() } + coVerify(exactly = 0) { editorService.saveFile(any()) } } // --- Content changing while the approval dialog is open ----------------- diff --git a/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/EditorBufferApplierTest.kt b/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/EditorBufferApplierTest.kt new file mode 100644 index 00000000..bd8c62a1 --- /dev/null +++ b/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/EditorBufferApplierTest.kt @@ -0,0 +1,155 @@ +package com.itsaky.androidide.plugins.aicore.tool.handlers.edit + +import com.itsaky.androidide.plugins.services.IdeEditorService +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.asCoroutineDispatcher +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.io.File +import java.util.concurrent.Executors + +/** + * Unit tests for [EditorBufferApplier] — specifically that the save targets the *file* and never the + * focused tab. Saving by focus persisted whichever tab the user happened to be in (ADFA-5215), so the + * negative verifications on `openFile`/`saveCurrentFile` below are the regression lock. + */ +class EditorBufferApplierTest { + + private companion object { + const val EDITOR_THREAD = "test-editor" + } + + private lateinit var editorService: IdeEditorService + private lateinit var applier: EditorBufferApplier + + private val target = File("/project/A.kt") + private val buffer = "line0\nline1\nline2\n" + + @Before + fun setup() { + editorService = mockk(relaxed = true) + every { editorService.getFileContent(target) } returns buffer + every { editorService.replaceRange(any(), any(), any()) } returns true + coEvery { editorService.saveFile(any()) } returns true + applier = EditorBufferApplier(editorService, Dispatchers.Unconfined) + } + + /** The running thread, without the ` @coroutine#N` suffix the test JVM's debug mode appends. */ + private fun currentThreadName() = Thread.currentThread().name.substringBefore(" @") + + private fun apply( + matched: String = buffer, + applier: EditorBufferApplier = this.applier, + ): EditorBufferApplier.Outcome = + runBlocking { + applier.apply( + file = target, + displayPath = "A.kt", + matched = matched, + updated = matched.replace("line1", "LINE1"), + oldString = "line1", + newString = "LINE1", + occurrences = 1, + ) + } + + @Test + fun givenAnUnfocusedFile_whenApplied_thenItIsSavedByFileWithoutStealingFocus() { + // The user is looking at another tab; the edit must neither follow nor move focus. + every { editorService.getCurrentFile() } returns File("/project/B.kt") + + val outcome = apply() + + assertEquals(EditorBufferApplier.Outcome.Applied(saved = true), outcome) + coVerify { editorService.saveFile(target) } + verify(exactly = 0) { editorService.openFile(any()) } + verify(exactly = 0) { editorService.saveCurrentFile() } + } + + @Test + fun givenTheHostSavesTheFile_whenApplied_thenTheOutcomeReportsSaved() { + assertEquals(EditorBufferApplier.Outcome.Applied(saved = true), apply()) + } + + @Test + fun givenTheHostCannotSaveTheFile_whenApplied_thenTheEditStillCountsAsApplied() { + coEvery { editorService.saveFile(any()) } returns false + + assertEquals(EditorBufferApplier.Outcome.Applied(saved = false), apply()) + } + + @Test + fun givenSaveDeniedByPermissions_whenApplied_thenItIsReportedAsUnsavedNotPropagated() { + // Letting a permission miss escape would abort the turn over an edit already in the buffer. + coEvery { editorService.saveFile(any()) } throws SecurityException("FILESYSTEM_WRITE denied") + + assertEquals(EditorBufferApplier.Outcome.Applied(saved = false), apply()) + } + + @Test + fun givenSaveFailingUnexpectedly_whenApplied_thenTheErrorIsNotSwallowed() { + // Only SecurityException means "unsaved"; a defect must not pass as an applied-but-unsaved edit. + coEvery { editorService.saveFile(any()) } throws IllegalStateException("host defect") + + assertThrows(IllegalStateException::class.java) { apply() } + } + + @Test + fun givenAnEditorDispatcher_whenApplied_thenOnlyTheBufferEditRunsOnIt() { + // saveFile is a suspending host call that reaches the editor thread on its own; re-dispatching + // it here - or saving inside the editor block - would put the applier back in that business. + val editorExecutor = Executors.newSingleThreadExecutor { Thread(it, EDITOR_THREAD) } + var replaceThread: String? = null + var saveThread: String? = null + every { editorService.replaceRange(any(), any(), any()) } answers { + replaceThread = currentThreadName() + true + } + coEvery { editorService.saveFile(any()) } answers { + saveThread = currentThreadName() + true + } + + try { + val outcome = apply( + applier = EditorBufferApplier(editorService, editorExecutor.asCoroutineDispatcher()), + ) + + assertEquals(EditorBufferApplier.Outcome.Applied(saved = true), outcome) + assertEquals(EDITOR_THREAD, replaceThread) + assertEquals(currentThreadName(), saveThread) + } finally { + editorExecutor.shutdownNow() + } + } + + @Test + fun givenAStaleBuffer_whenApplied_thenNothingIsEditedOrSaved() { + every { editorService.getFileContent(target) } returns "the user typed this instead\n" + + val outcome = apply() + + assertTrue("Expected a refusal, got: $outcome", outcome is EditorBufferApplier.Outcome.Failed) + verify(exactly = 0) { editorService.replaceRange(any(), any(), any()) } + coVerify(exactly = 0) { editorService.saveFile(any()) } + } + + @Test + fun givenReplaceRangeRejectingTheEdit_whenApplied_thenNothingIsSaved() { + every { editorService.replaceRange(any(), any(), any()) } returns false + + val outcome = apply() + + assertTrue("Expected a refusal, got: $outcome", outcome is EditorBufferApplier.Outcome.Failed) + coVerify(exactly = 0) { editorService.saveFile(any()) } + } +}