From 4ababd11400ccd4e70a481b311618cff78ffe926 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 10:40:09 -0700 Subject: [PATCH 01/11] ADFA-6835 | Show the file's closest lines when edit_file rejects an edit A model that guesses old_string and is told only "not found" (or "identical to new_string") guesses again. The rejection now lists up to five lines of the file that share the most words with the guess, exact text with spaces visible. An identical-strings rejection is decided after the file is read, so it can carry the lines too. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- .../aicore/tool/handlers/EditFileHandler.kt | 14 ++-- .../tool/handlers/edit/NearMatchLines.kt | 54 +++++++++++++++ .../tool/handlers/EditFileHandlerTest.kt | 41 ++++++++++++ .../tool/handlers/edit/NearMatchLinesTest.kt | 65 +++++++++++++++++++ 4 files changed, 170 insertions(+), 4 deletions(-) create mode 100644 plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt create mode 100644 plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt index f5537013..0207ca08 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt @@ -10,6 +10,7 @@ import com.itsaky.androidide.plugins.aicore.tool.handlers.edit.AtomicFileWriter import com.itsaky.androidide.plugins.aicore.tool.handlers.edit.EditTargetResolver import com.itsaky.androidide.plugins.aicore.tool.handlers.edit.EditorBufferApplier import com.itsaky.androidide.plugins.aicore.tool.handlers.edit.FileTextMatcher +import com.itsaky.androidide.plugins.aicore.tool.handlers.edit.NearMatchLines import com.itsaky.androidide.plugins.aicore.logging.AgentTrace import com.itsaky.androidide.plugins.aicore.tool.parseToolBoolean import com.itsaky.androidide.plugins.services.IdeEditorService @@ -236,9 +237,6 @@ class EditFileHandler( "$ARG_OLD/$ARG_NEW must be under $MAX_ARG_CHARS characters — edit a smaller region" ) } - if (oldString == newString) { - return reject(identicalArgsRejection(oldString)) - } val target = when (val resolution = EditTargetResolver.resolve(filePath)) { is EditTargetResolver.Target.Rejected -> { @@ -281,6 +279,13 @@ class EditFileHandler( ) } + // After the read, so the rejection can show the lines the model should have copied. + if (oldString == newString) { + return reject( + identicalArgsRejection(oldString) + NearMatchLines.render(NearMatchLines.find(original, oldString)) + ) + } + val match = FileTextMatcher.match(original, oldString, newString) if (match is FileTextMatcher.Match.NotFound) { // The dominant local-model failure: whitespace or escaping drift from the real file. @@ -293,7 +298,8 @@ class EditFileHandler( } return reject( "$ARG_OLD not found in $resolvedPath — read the file first and copy the target " + - "text exactly, including indentation and line breaks" + "text exactly, including indentation and line breaks" + + NearMatchLines.render(NearMatchLines.find(original, oldString)) ) } val found = match as FileTextMatcher.Match.Found diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt new file mode 100644 index 00000000..d000cb0c --- /dev/null +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt @@ -0,0 +1,54 @@ +package com.itsaky.androidide.plugins.aicore.tool.handlers.edit + +/** + * Finds the lines of a file that a failed `old_string` was probably aiming at, so a rejection can + * show them. A small model that guesses `old_string` and is told only "not found" guesses again; + * shown the real line, it can copy it. Matching is by shared words, never by regex, for the same + * reason as [FileTextMatcher]. + */ +object NearMatchLines { + + /** One candidate line, with its 1-based number and its exact text (no line ending). */ + data class Line(val number: Int, val text: String) + + private const val MAX_LINES = 5 + private const val MAX_LINE_CHARS = 200 + private const val MIN_WORD_CHARS = 3 + private val WORD = Regex("[A-Za-z0-9_]{$MIN_WORD_CHARS,}") + + /** + * The lines of [text] sharing the most distinct words with [snippet]. + * @param text the file contents the edit was matched against. + * @param snippet the `old_string` that was not found or changed nothing. + * @return up to [MAX_LINES] lines, best match first and file order among equals; empty when + * the snippet has no word of [MIN_WORD_CHARS] or more characters or no line shares one. + */ + fun find(text: String, snippet: String): List { + val words = WORD.findAll(snippet).map { it.value.lowercase() }.distinct().toList() + if (words.isEmpty()) return emptyList() + return text.lineSequence() + .mapIndexed { index, raw -> Line(index + 1, raw.trimEnd('\r').take(MAX_LINE_CHARS)) } + .filter { it.text.isNotBlank() } + .map { line -> + val lower = line.text.lowercase() + line to words.count { lower.contains(it) } + } + .filter { (_, score) -> score > 0 } + .sortedByDescending { (_, score) -> score } + .take(MAX_LINES) + .map { (line, _) -> line } + .toList() + } + + /** + * Renders [lines] for a rejection message, keeping leading and trailing spaces visible. + * @param lines the result of [find]. + * @return the text to append to a rejection, or an empty string when there are no lines. + */ + fun render(lines: List): String { + if (lines.isEmpty()) return "" + val body = lines.joinToString("\n") { "line ${it.number}: \"${it.text}\"" } + return "\nClosest lines in the file (the exact text is between the outer double quotes, " + + "spaces included):\n$body" + } +} diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt index a547a3e8..e669058b 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt @@ -642,6 +642,47 @@ class EditFileHandlerTest { assertTrue(result!!.message.contains("identical")) } + @Test + fun givenIdenticalStringsThatGuessAtALine_whenValidated_thenTheRejectionShowsTheRealLine() = runBlocking { + // The ADFA-6835 failure: the model sent its own new text as old_string and never read the file. + createFile("strings.xml", "\n Build Apps Anywhere \n\n") + + val result = rejectionOf( + mapOf( + "file_path" to "strings.xml", + "old_string" to "app_name = \"Block Stacker\"", + "new_string" to "app_name = \"Block Stacker\"", + ) + ) + + assertTrue(result!!.message.contains("identical")) + assertTrue(result.message.contains("""line 2: " Build Apps Anywhere """")) + } + + @Test + fun givenAGuessNotInTheFile_whenValidated_thenTheRejectionShowsTheClosestLines() = runBlocking { + createFile("Main.kt", "fun main() {\n println(\"hello world\")\n}\n") + + val result = rejectionOf( + mapOf("file_path" to "Main.kt", "old_string" to "println(\"hi\")", "new_string" to "println(\"bye\")") + ) + + assertTrue(result!!.message.contains("not found")) + assertTrue(result.message.contains("line 2: \" println(\"hello world\")\"")) + } + + @Test + fun givenAGuessSharingNoWordWithTheFile_whenValidated_thenTheRejectionIsUnchanged() = runBlocking { + createFile("Main.kt", "fun main() {}\n") + + val result = rejectionOf( + mapOf("file_path" to "Main.kt", "old_string" to "zzz_missing", "new_string" to "yyy") + ) + + assertTrue(result!!.message.contains("not found")) + assertTrue(!result.message.contains("Closest lines")) + } + @Test fun givenAHallucinatedPath_whenValidated_thenItIsRejectedWithoutPromptingTheUser() = runBlocking { val result = rejectionOf( diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt new file mode 100644 index 00000000..62913af1 --- /dev/null +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -0,0 +1,65 @@ +package com.itsaky.androidide.plugins.aicore.tool.handlers.edit + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test + +class NearMatchLinesTest { + + private val strings = "\n" + + " Build Apps Anywhere \n" + + " Hello\n" + + "\n" + + @Test + fun givenAGuessSharingAWordWithOneLine_whenFound_thenThatLineComesBackExactly() { + val lines = NearMatchLines.find(strings, "app_name = \"Block Stacker\"") + + assertEquals( + NearMatchLines.Line(2, " Build Apps Anywhere "), + lines.first(), + ) + } + + @Test + fun givenSeveralLines_whenFound_thenTheOnesSharingMostWordsComeFirstInFileOrder() { + val text = "alpha\nalpha beta\nbeta\nalpha beta\n" + + val lines = NearMatchLines.find(text, "alpha beta") + + assertEquals(listOf(2, 4, 1, 3), lines.map { it.number }) + } + + @Test + fun givenManyMatchingLines_whenFound_thenTheListIsCapped() { + val text = (1..50).joinToString("\n") { "val name$it = foo" } + + assertEquals(5, NearMatchLines.find(text, "foo").size) + } + + @Test + fun givenALongLine_whenFound_thenItIsCut() { + val text = "foo " + "x".repeat(1_000) + + assertEquals(200, NearMatchLines.find(text, "foo").single().text.length) + } + + @Test + fun givenCrlfText_whenFound_thenNoCarriageReturnIsKept() { + assertEquals("foo bar", NearMatchLines.find("foo bar\r\nbaz\r\n", "foo").single().text) + } + + @Test + fun givenNoSharedWordOrNoWordAtAll_whenFound_thenNothingComesBack() { + assertTrue(NearMatchLines.find(strings, "zzz_nothing").isEmpty()) + assertTrue(NearMatchLines.find(strings, "= \"\"").isEmpty()) + assertEquals("", NearMatchLines.render(emptyList())) + } + + @Test + fun givenLines_whenRendered_thenTrailingSpacesStayVisibleInsideTheQuotes() { + val rendered = NearMatchLines.render(listOf(NearMatchLines.Line(7, "a "))) + + assertTrue(rendered.contains("line 7: \"a \"")) + } +} From 0ea705d9dd947251dc473f5d51f7de67cf979ebd Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 10:40:09 -0700 Subject: [PATCH 02/11] ADFA-6835 | Tell a run its file change failed instead of ending on "done" When a run answers while its last file change failed (not user-denied) and none has succeeded since, it is told once that nothing was changed and to read and retry or say so. Answering again ends the run ABANDONED, not COMPLETED. The wording is in agent_loop.yml. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- .../src/main/assets/prompts/agent_loop.yml | 7 ++ .../aicore/prompt/ToolResultsPrompt.kt | 17 ++++ .../aicore/prompt/config/AgentPromptConfig.kt | 3 + .../prompt/config/AgentPromptConfigParser.kt | 1 + .../plugins/aicore/tool/AgentLoop.kt | 37 ++++++- .../aicore/viewmodel/AgentRunReporter.kt | 4 + .../plugins/aicore/viewmodel/ChatViewModel.kt | 1 + .../plugins/aicore/tool/AgentLoopTest.kt | 97 +++++++++++++++++++ 8 files changed, 162 insertions(+), 5 deletions(-) diff --git a/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml b/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml index a4818c86..78f6a7da 100644 --- a/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml +++ b/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml @@ -30,6 +30,13 @@ agent_loop: Your reply called no tool, so the run is still open. If every part of the request is now answered, call the "{{TERMINAL_TOOL}}" tool with a one-line summary — do not repeat what you already wrote. Otherwise do the next part now. + # Sent once when a run tries to finish while its last attempt to change a file failed and no change + # has succeeded since: a small model tends to answer as if the change had been made. + unapplied_write: >- + Nothing was changed: your last attempt to change a file failed, and no change has succeeded + since. Do not tell the user it was done. Read the file with read_file, then repeat the change + with the text copied exactly from it; or, if you cannot, call the "{{TERMINAL_TOOL}}" tool and + say plainly that the change was not made. # Sent once when a run that had to check the web first answered without searching; TOOL is the # search tool. A backend that forces the call never sends it (see VerificationPolicy). required_tool: >- diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt index e65fd913..fea3a0cf 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt @@ -31,6 +31,9 @@ class ToolResultsPrompt( /** The turn sent after a reply that ran tools and ended without the terminal tool. */ suspend fun unfinished(): String = renderUnfinished(config.config(), terminalTool) + /** The turn sent after a reply that tried to finish while its last file change had failed. */ + suspend fun unappliedWrite(): String = renderUnappliedWrite(config.config(), terminalTool) + /** The turn sent after a reply that answered before calling [tool], which the run had to call first. */ suspend fun requiredTool(tool: String): String = renderRequiredTool(config.config(), terminalTool, tool) @@ -93,6 +96,19 @@ class ToolResultsPrompt( mapOf(PromptVariables.TERMINAL_TOOL to terminalTool), ) + /** + * Renders the turn that tells a run its last attempt to change a file failed. + * + * @param config the loaded prompt config. + * @param terminalTool the name of the tool that ends a run by answering the user. + * @return the turn to add to the transcript. + */ + fun renderUnappliedWrite(config: AgentPromptConfig, terminalTool: String): String = + PromptTemplateEngine.render( + config.agentLoop.unappliedWrite, + mapOf(PromptVariables.TERMINAL_TOOL to terminalTool), + ) + /** * Renders the turn that asks a run which answered too early to call [tool] first. * @@ -117,6 +133,7 @@ class ToolResultsPrompt( val renders: List<() -> String> = CHECK_BATCHES.map { (calls, results) -> { render(config, CHECK_TERMINAL_TOOL, CHECK_CHAR_LIMIT, calls, results) } } + { renderUnfinished(config, CHECK_TERMINAL_TOOL) } + + { renderUnappliedWrite(config, CHECK_TERMINAL_TOOL) } + { renderRequiredTool(config, CHECK_TERMINAL_TOOL, CHECK_REQUIRED_TOOL) } return renders.mapNotNull { check -> try { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt index 83c0970b..0780eea8 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt @@ -111,6 +111,8 @@ data class AgentPromptConfig( * @property afterFailure what to do next otherwise. * @property unfinished the turn after a reply that ran tools and then ended without the * terminal tool. + * @property unappliedWrite the turn after a reply that tried to finish while the last attempt to + * change a file had failed and none had succeeded since. * @property requiredTool the turn after a reply that answered before calling the tool the run * had to call first, such as a web search before a code review. */ @@ -121,6 +123,7 @@ data class AgentPromptConfig( val afterSuccess: PromptText, val afterFailure: PromptText, val unfinished: PromptText, + val unappliedWrite: PromptText, val requiredTool: PromptText, ) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfigParser.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfigParser.kt index c614e618..848c9f3d 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfigParser.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfigParser.kt @@ -74,6 +74,7 @@ object AgentPromptConfigParser : PromptConfigParser { afterSuccess = text("after_success"), afterFailure = text("after_failure"), unfinished = text("unfinished"), + unappliedWrite = text("unapplied_write"), requiredTool = text("required_tool"), ) }, diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index 62f0183b..9cc3b2ed 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -15,6 +15,8 @@ import com.itsaky.androidide.plugins.services.LlmInferenceService.ChatMessage.Ro * [terminalTool]; null ends the run on that reply instead. * @param requiredToolTurn words the turn sent when a run told to call a tool first tries to finish * without it; receives the tool's name. Null never asks. + * @param unappliedWriteTurn words the turn sent when a run answers while its last attempt to change + * a file failed and none has succeeded since; null lets that answer end the run. */ class AgentLoop( private val formatToolResults: ToolResultsFormatter, @@ -27,6 +29,7 @@ class AgentLoop( private val terminalTool: String? = null, private val unfinishedTurn: (suspend () -> String)? = null, private val requiredToolTurn: (suspend (String) -> String)? = null, + private val unappliedWriteTurn: (suspend () -> String)? = null, ) { companion object { @@ -156,6 +159,15 @@ class AgentLoop( * @param tool the tool the run had to call. */ suspend fun onRequiredToolSkipped(turn: Int, tool: String) {} + + /** + * The run answered while its last attempt to change a file had failed and no change had + * succeeded since, so it was told the change was not made rather than being ended on a + * reply that probably says it was. + * + * @param turn 1-based turn index. + */ + suspend fun onUnappliedWriteAnswer(turn: Int) {} } /** Why the loop stopped. */ @@ -227,6 +239,9 @@ class AgentLoop( // Once a tool has run, only the terminal tool finishes the run; see [unfinishedTurn]. var toolsRan = false var askedToFinish = false + // True from a failed (not user-denied) file change until a later one succeeds. + var writeUnapplied = false + var warnedOfUnappliedWrite = false // Cleared once the tool runs or has been asked for, so a model that refuses it still stops. var requiredPending = requiredTool != null && requiredToolTurn != null while (turn < maxIterations) { @@ -283,12 +298,21 @@ class AgentLoop( askForRequiredTool(turn, requiredTool!!, history, events) continue } - terminalTool?.let { tt -> - val terminal = calls.firstOrNull { isTerminalToolName(it.name, tt) } - if (terminal != null && realCalls.isEmpty()) { - events.onFinalAnswer(turn, respondMessageOf(terminal.args).orEmpty()) - return Result(turn, StopReason.COMPLETED) + val terminal = terminalTool?.let { tt -> calls.firstOrNull { isTerminalToolName(it.name, tt) } } + if (terminal != null && realCalls.isEmpty()) { + if (writeUnapplied && unappliedWriteTurn != null) { + if (!warnedOfUnappliedWrite) { + warnedOfUnappliedWrite = true + events.onUnappliedWriteAnswer(turn) + history.add(ChatMessage(Role.USER, unappliedWriteTurn.invoke())) + continue + } + // Told once and answered again: the user's change is still not made. + events.onAbandonedAfterFailure(turn) + return Result(turn, StopReason.ABANDONED) } + events.onFinalAnswer(turn, respondMessageOf(terminal.args).orEmpty()) + return Result(turn, StopReason.COMPLETED) } when (progress.inspect(realCalls)) { @@ -312,6 +336,9 @@ class AgentLoop( toolsRan = true askedToFinish = false progress.recordResults(results) + realCalls.zip(results).filter { (call, _) -> changesPaths(call) }.forEach { (_, result) -> + if (result.success) writeUnapplied = false else if (!result.denied) writeUnapplied = true + } events.onToolResults(turn, realCalls, results) history.add(ChatMessage(Role.USER, formatToolResults.format(realCalls, results))) } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt index 78a2e8a5..03b8e809 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt @@ -86,6 +86,10 @@ internal class AgentRunReporter( onAskedToFinish() } + override suspend fun onUnappliedWriteAnswer(turn: Int) { + AgentTrace.refusal("LOOP", "turn=$turn", "answered with a failed file change outstanding; told it was not made") + } + // Reaching this means the backend did not force the call; see VerificationPolicy. override suspend fun onRequiredToolSkipped(turn: Int, tool: String) { AgentTrace.stage("VERIFY", "turn=$turn asked-for=$tool (answered without it)") diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index d52a838c..da740bcf 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -226,6 +226,7 @@ class ChatViewModel( terminalTool = RESPOND_TOOL, unfinishedTurn = toolResultsPrompt::unfinished, requiredToolTurn = toolResultsPrompt::requiredTool, + unappliedWriteTurn = toolResultsPrompt::unappliedWrite, ) val toolExecutionTracker = ToolExecutionTracker() diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index e3fc09b3..79f05c4d 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -951,8 +951,105 @@ class AgentLoopTest { assertTrue(results.contains("THE SOURCES")) } + /** [finishingLoop], also telling a run that answers over a failed file change it was not made. */ + private fun writingLoop() = AgentLoop( + formatToolResults = ToolResultsPrompt({ shippedConfig }, "respond"), + terminalTool = "respond", + unfinishedTurn = { UNFINISHED }, + unappliedWriteTurn = { UNAPPLIED }, + ) + + /** Runs [writingLoop] with `edit_file` as the one tool that changes files; results come from [outcomes]. */ + private suspend fun runWriting( + replies: List, + outcomes: Map>, + ): Triple, List> { + val model = ScriptedModel(replies) + val used = mutableMapOf() + val events = mutableListOf() + val history = mutableListOf(ChatMessage(Role.USER, "change the app name")) + val result = writingLoop().run( + history = history, + generate = model::generate, + executeTools = { calls -> + calls.map { call -> + val n = used.getOrDefault(call.name, 0) + used[call.name] = n + 1 + outcomes.getValue(call.name).let { it.getOrElse(n) { _ -> it.last() } } + } + }, + changesPaths = { it.name == "edit_file" }, + events = object : AgentLoop.Events { + override suspend fun onUnappliedWriteAnswer(turn: Int) { events += "unapplied" } + override suspend fun onFinalAnswer(turn: Int, message: String) { events += "final" } + override suspend fun onAbandonedAfterFailure(turn: Int) { events += "abandoned" } + }, + ) + return Triple(result, history, events) + } + + private val failedEdit = ToolResult.failure("old_string not found") + private val appliedEdit = ToolResult.success("edited") + + @Test + fun givenAnAnswerOverAFailedEdit_whenTheModelThenEditsSuccessfully_thenItIsToldOnceAndCompletes() = runTest { + val (result, history, events) = runWriting( + listOf(toolCall("edit_file"), respond, toolCall("edit_file"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("unapplied", "final"), events) + assertEquals(1, history.count { it.content == UNAPPLIED }) + } + + @Test + fun givenAnAnswerOverAFailedEdit_whenTheModelAnswersAgainWithoutEditing_thenTheRunIsAbandonedNotCompleted() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file"), respond, respond), + mapOf("edit_file" to listOf(failedEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "abandoned"), events) + } + + @Test + fun givenAnEditTheUserDenied_whenTheModelAnswers_thenItIsNotToldTheChangeFailed() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file"), respond), + mapOf("edit_file" to listOf(ToolResult.denied("User denied permission"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedReadOnlyCall_whenTheModelAnswers_thenItIsNotToldAChangeFailed() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("read_file"), respond), + mapOf("read_file" to listOf(ToolResult.failure("no such file"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedEditFollowedByASuccessfulOne_whenTheModelAnswers_thenNothingIsOutstanding() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file", "a"), toolCall("edit_file", "b"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + private companion object { const val UNFINISHED = "finish or carry on" + const val UNAPPLIED = "the change was not made" const val REQUIRED = "call first:" } } From f4e31966f0214c3b090fac39b9fe01aa41b4405a Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 11:26:34 -0700 Subject: [PATCH 03/11] ADFA-6835 | Tell the user when a failed file change was never redone After being told its edit failed, a model re-read the file and the repeat-after-success rule then ended the run COMPLETED with its false "has been changed" reply standing. Now a run that ends with a failed change outstanding, by answering twice or by repeating a look, is ABANDONED and shows a notice that the change was not made. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- .../androidide/plugins/aicore/tool/AgentLoop.kt | 15 ++++++++++++++- .../aicore/viewmodel/AgentRunReporter.kt | 8 ++++++++ .../plugins/aicore/viewmodel/ChatViewModel.kt | 3 +++ plugins/AI-Core/src/main/res/values/strings.xml | 1 + .../plugins/aicore/tool/AgentLoopTest.kt | 17 +++++++++++++++-- 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index 9cc3b2ed..315ac8dc 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -168,6 +168,14 @@ class AgentLoop( * @param turn 1-based turn index. */ suspend fun onUnappliedWriteAnswer(turn: Int) {} + + /** + * The run ended with a failed file change still outstanding after it had been told, so + * whatever the last reply says, the change was not made. + * + * @param turn 1-based turn index. + */ + suspend fun onChangeNotMade(turn: Int) {} } /** Why the loop stopped. */ @@ -308,7 +316,7 @@ class AgentLoop( continue } // Told once and answered again: the user's change is still not made. - events.onAbandonedAfterFailure(turn) + events.onChangeNotMade(turn) return Result(turn, StopReason.ABANDONED) } events.onFinalAnswer(turn, respondMessageOf(terminal.args).orEmpty()) @@ -321,6 +329,11 @@ class AgentLoop( return Result(turn, StopReason.CYCLING) } ToolCallProgressGuard.Verdict.ASSUME_COMPLETE -> { + // A repeated look is not completion while the change the user asked for failed. + if (writeUnapplied) { + events.onChangeNotMade(turn) + return Result(turn, StopReason.ABANDONED) + } events.onRepeatAfterSuccess(turn) return Result(turn, StopReason.COMPLETED) } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt index 03b8e809..7232d30d 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt @@ -34,6 +34,9 @@ internal class AgentRunReporter( /** The model kept re-issuing one batch that was not working. */ suspend fun repeatedCalls() + /** The run ended with a failed file change never redone, whatever the last reply says. */ + suspend fun changeNotMade() + /** The model kept circling actions it had already taken. */ suspend fun noProgress() } @@ -90,6 +93,11 @@ internal class AgentRunReporter( AgentTrace.refusal("LOOP", "turn=$turn", "answered with a failed file change outstanding; told it was not made") } + override suspend fun onChangeNotMade(turn: Int) { + AgentTrace.refusal("LOOP", "turn=$turn", "ended with a failed file change never redone") + notices.changeNotMade() + } + // Reaching this means the backend did not force the call; see VerificationPolicy. override suspend fun onRequiredToolSkipped(turn: Int, tool: String) { AgentTrace.stage("VERIFY", "turn=$turn asked-for=$tool (answered without it)") diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index da740bcf..36b95bd5 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -2036,6 +2036,9 @@ class ChatViewModel( override suspend fun repeatedCalls() = addSystemMessage(str(R.string.agent_repeated_calls), MessageStatus.SENT) + override suspend fun changeNotMade() = + addSystemMessage(str(R.string.agent_change_not_made), MessageStatus.SENT) + override suspend fun noProgress() = addSystemMessage(str(R.string.agent_no_progress), MessageStatus.SENT) } diff --git a/plugins/AI-Core/src/main/res/values/strings.xml b/plugins/AI-Core/src/main/res/values/strings.xml index cad1adc1..4e625fce 100644 --- a/plugins/AI-Core/src/main/res/values/strings.xml +++ b/plugins/AI-Core/src/main/res/values/strings.xml @@ -219,6 +219,7 @@ Reached the %1$d-step limit. Ask me to continue if the task isn\'t finished. Stopped: the model kept requesting the same action. Try rephrasing your request. + The change was not made: the file edit failed and the assistant did not manage to redo it, whatever its last message says. Try again, or ask for a smaller change. Stopped: the model kept redoing actions it had already taken instead of making progress. Try rephrasing your request, or asking for a smaller step. Earlier messages were summarized to stay within the model\'s limit. Earlier messages could not be summarized, so only the most recent ones were kept to stay within the model\'s limit. diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index 79f05c4d..df255e82 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -982,7 +982,7 @@ class AgentLoopTest { events = object : AgentLoop.Events { override suspend fun onUnappliedWriteAnswer(turn: Int) { events += "unapplied" } override suspend fun onFinalAnswer(turn: Int, message: String) { events += "final" } - override suspend fun onAbandonedAfterFailure(turn: Int) { events += "abandoned" } + override suspend fun onChangeNotMade(turn: Int) { events += "not-made" } }, ) return Triple(result, history, events) @@ -1011,7 +1011,20 @@ class AgentLoopTest { ) assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) - assertEquals(listOf("unapplied", "abandoned"), events) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAnAnswerOverAFailedEdit_whenTheModelOnlyRepeatsALookAfterTheWarning_thenTheRunIsAbandonedNotAssumedComplete() = runTest { + // Seen on a device: told the edit failed, the model re-read the file, and the repeat-after- + // success rule then ended the run as COMPLETED with its false "changed" answer standing. + val (result, _, events) = runWriting( + listOf(toolCall("edit_file"), toolCall("read_file"), respond, toolCall("read_file")), + mapOf("edit_file" to listOf(failedEdit), "read_file" to listOf(ToolResult.success("lines"))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) } @Test From aaecc1389a8fd20c3c97ca6a737e2e4eaa0b5e1d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 15:39:31 -0700 Subject: [PATCH 04/11] ADFA-6835 | Track a failed file change per path until it is redone Review of #157 found six ways the run-wide unapplied-write flag gave a false result: - A success on one file cleared a failure on another in the same batch. - The warning was never re-armed, so a second failure got none. - A prose answer skipped the check; only "respond" was guarded. - Any project-changing tool set the flag, so a failed shell command or add_dependency was reported as a failed file edit. - The same signal cleared it, so a shell `cat` hid a failed edit. - A user denial left it set, sending the model back to the change the user had refused. Failures are now kept per path, and only from the tools that write a file's content (edit_file, create_file, update_file; new ToolHandler.writesFiles). A success or a denial clears that path; a call that names no path is cleared by any later successful write. The warning re-arms once nothing is outstanding, and prose answers get the same check as "respond". The prompt and the user notice no longer say that nothing at all was changed. Each new AgentLoopTest case fails against the previous AgentLoop. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VKUthHVokuWKgP3ACLBii7 --- .../src/main/assets/prompts/agent_loop.yml | 8 +- .../plugins/aicore/tool/AgentLoop.kt | 89 +++++++++++++++---- .../plugins/aicore/tool/ToolHandler.kt | 8 ++ .../aicore/tool/handlers/CreateFileHandler.kt | 1 + .../aicore/tool/handlers/EditFileHandler.kt | 1 + .../aicore/tool/handlers/UpdateFileHandler.kt | 1 + .../plugins/aicore/viewmodel/ChatViewModel.kt | 3 + .../AI-Core/src/main/res/values/strings.xml | 2 +- .../plugins/aicore/tool/AgentLoopTest.kt | 81 ++++++++++++++++- 9 files changed, 169 insertions(+), 25 deletions(-) diff --git a/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml b/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml index 78f6a7da..d94fb628 100644 --- a/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml +++ b/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml @@ -30,11 +30,11 @@ agent_loop: Your reply called no tool, so the run is still open. If every part of the request is now answered, call the "{{TERMINAL_TOOL}}" tool with a one-line summary — do not repeat what you already wrote. Otherwise do the next part now. - # Sent once when a run tries to finish while its last attempt to change a file failed and no change - # has succeeded since: a small model tends to answer as if the change had been made. + # Sent once when a run tries to finish while a file change it attempted failed and has not been + # redone: a small model tends to answer as if the change had been made. unapplied_write: >- - Nothing was changed: your last attempt to change a file failed, and no change has succeeded - since. Do not tell the user it was done. Read the file with read_file, then repeat the change + A file change you tried failed and has not been redone, so that file is unchanged. Do not tell + the user it was done. Read the file with read_file, then repeat the change with the text copied exactly from it; or, if you cannot, call the "{{TERMINAL_TOOL}}" tool and say plainly that the change was not made. # Sent once when a run that had to check the web first answered without searching; TOOL is the diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index 315ac8dc..fedaf3c5 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -15,8 +15,8 @@ import com.itsaky.androidide.plugins.services.LlmInferenceService.ChatMessage.Ro * [terminalTool]; null ends the run on that reply instead. * @param requiredToolTurn words the turn sent when a run told to call a tool first tries to finish * without it; receives the tool's name. Null never asks. - * @param unappliedWriteTurn words the turn sent when a run answers while its last attempt to change - * a file failed and none has succeeded since; null lets that answer end the run. + * @param unappliedWriteTurn words the turn sent when a run answers while a file change it attempted + * failed and has not been redone; null lets that answer end the run. */ class AgentLoop( private val formatToolResults: ToolResultsFormatter, @@ -161,9 +161,9 @@ class AgentLoop( suspend fun onRequiredToolSkipped(turn: Int, tool: String) {} /** - * The run answered while its last attempt to change a file had failed and no change had - * succeeded since, so it was told the change was not made rather than being ended on a - * reply that probably says it was. + * The run answered while a file change it attempted had failed and had not been redone, so + * it was told the change was not made rather than being ended on a reply that probably says + * it was. * * @param turn 1-based turn index. */ @@ -220,6 +220,9 @@ class AgentLoop( * @param pathsOf the project paths a call names; the progress guard counts an earlier look at * one a later call rewrote as a new action again rather than as a repeat. * @param changesPaths whether a call rewrites what it names. + * @param writesFiles whether a call writes a file's content. Only these calls' failures count as a + * change not made, and only their successes as one redone: a shell command also changes the + * project, but its success says nothing about a failed edit. * @param canonicalName the registered name a call's tool resolves to, e.g. `Run_Shell_Command` to * `run_shell_command`; the name itself when none does. Everything keyed by tool name reads it. * @param requiredTool a tool the run must call before it may finish, e.g. a web search before a @@ -233,6 +236,7 @@ class AgentLoop( executeTools: suspend (List) -> List, pathsOf: (ToolCall) -> Set = { emptySet() }, changesPaths: (ToolCall) -> Boolean = { false }, + writesFiles: (ToolCall) -> Boolean = { false }, canonicalName: (String) -> String = { it }, requiredTool: String? = null, events: Events = object : Events {}, @@ -247,9 +251,7 @@ class AgentLoop( // Once a tool has run, only the terminal tool finishes the run; see [unfinishedTurn]. var toolsRan = false var askedToFinish = false - // True from a failed (not user-denied) file change until a later one succeeds. - var writeUnapplied = false - var warnedOfUnappliedWrite = false + val unapplied = UnappliedWrites() // Cleared once the tool runs or has been asked for, so a model that refuses it still stops. var requiredPending = requiredTool != null && requiredToolTurn != null while (turn < maxIterations) { @@ -279,6 +281,15 @@ class AgentLoop( askForRequiredTool(turn, requiredTool!!, history, events) continue } + // A prose answer can claim a failed change as readily as a terminal-tool one. + if (unapplied.outstanding && unappliedWriteTurn != null) { + if (unapplied.warnOnce()) { + warnOfUnappliedWrite(turn, history, events) + continue + } + events.onChangeNotMade(turn) + return Result(turn, StopReason.ABANDONED) + } // Asked once per stretch of prose, so a model that will not call it still stops. if (toolsRan && !askedToFinish && unfinishedTurn != null) { askedToFinish = true @@ -308,11 +319,9 @@ class AgentLoop( } val terminal = terminalTool?.let { tt -> calls.firstOrNull { isTerminalToolName(it.name, tt) } } if (terminal != null && realCalls.isEmpty()) { - if (writeUnapplied && unappliedWriteTurn != null) { - if (!warnedOfUnappliedWrite) { - warnedOfUnappliedWrite = true - events.onUnappliedWriteAnswer(turn) - history.add(ChatMessage(Role.USER, unappliedWriteTurn.invoke())) + if (unapplied.outstanding && unappliedWriteTurn != null) { + if (unapplied.warnOnce()) { + warnOfUnappliedWrite(turn, history, events) continue } // Told once and answered again: the user's change is still not made. @@ -330,7 +339,7 @@ class AgentLoop( } ToolCallProgressGuard.Verdict.ASSUME_COMPLETE -> { // A repeated look is not completion while the change the user asked for failed. - if (writeUnapplied) { + if (unapplied.outstanding) { events.onChangeNotMade(turn) return Result(turn, StopReason.ABANDONED) } @@ -349,8 +358,8 @@ class AgentLoop( toolsRan = true askedToFinish = false progress.recordResults(results) - realCalls.zip(results).filter { (call, _) -> changesPaths(call) }.forEach { (_, result) -> - if (result.success) writeUnapplied = false else if (!result.denied) writeUnapplied = true + realCalls.zip(results).filter { (call, _) -> writesFiles(call) }.forEach { (call, result) -> + unapplied.record(pathsOf(call), result) } events.onToolResults(turn, realCalls, results) history.add(ChatMessage(Role.USER, formatToolResults.format(realCalls, results))) @@ -360,6 +369,54 @@ class AgentLoop( return Result(turn, StopReason.MAX_ITERATIONS) } + /** Records the answer as not finishing the run and tells it a file change it attempted was not made. */ + private suspend fun warnOfUnappliedWrite( + turn: Int, + history: MutableList, + events: Events, + ) { + events.onUnappliedWriteAnswer(turn) + history.add(ChatMessage(Role.USER, unappliedWriteTurn!!.invoke())) + } + + /** + * The file changes a run attempted that failed and have not been redone, keyed by the paths each + * call names, so a success on one file does not hide a failure on another. A call that names no + * path cannot be matched to a file, so any later successful write clears it. + */ + private class UnappliedWrites { + private val paths = mutableSetOf() + private var warned = false + + val outstanding: Boolean get() = paths.isNotEmpty() + + /** A denial drops the call's paths: the user has decided that change should not be made. */ + fun record(named: Set, result: ToolResult) { + val keys = named.ifEmpty { setOf(UNNAMED) } + when { + result.success -> { + paths.removeAll(keys) + paths.remove(UNNAMED) + } + result.denied -> paths.removeAll(keys) + else -> paths.addAll(keys) + } + // Re-armed once nothing is outstanding, so a later failure gets its own warning. + if (paths.isEmpty()) warned = false + } + + /** True the first time a run answers over [outstanding] changes since it was last re-armed. */ + fun warnOnce(): Boolean { + if (warned) return false + warned = true + return true + } + + private companion object { + const val UNNAMED = "" + } + } + /** Records the reply as not finishing the run and asks for [tool] in the next user turn. */ private suspend fun askForRequiredTool( turn: Int, diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt index bfa719a1..9c4a74d9 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt @@ -40,6 +40,14 @@ interface ToolHandler { val mutatesProject: Boolean get() = false + /** + * Whether this tool writes a file's content, which is what tells a run whether a change it set out + * to make was made. Narrower than [mutatesProject]: a shell command or a dependency change also + * changes the project, but its failure is not a failed file change, nor its success a redone one. + */ + val writesFiles: Boolean + get() = false + /** * JSON Schema for the arguments, in the shape the backend's tool definitions take. Empty means * untyped, flat string arguments, which is what the tool-call protocol supports today. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt index 4928dd81..77e63fdb 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt @@ -23,6 +23,7 @@ class CreateFileHandler( ) override val requiresApproval = true // Requires approval for file creation override val mutatesProject = true + override val writesFiles = true override val pathArgs = listOf("file_path") override suspend fun execute(args: Map): ToolResult { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt index 0207ca08..8e0f9dd4 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt @@ -49,6 +49,7 @@ class EditFileHandler( override val requiresApproval = true override val approvalPreview = ApprovalPreview.EDIT override val mutatesProject = true + override val writesFiles = true override val pathArgs = listOf(ARG_PATH) override val argAliases = mapOf( "old" to ARG_OLD, diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt index d93352cb..3cda207c 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt @@ -23,6 +23,7 @@ class UpdateFileHandler( ) override val requiresApproval = true // Requires approval for file modification override val mutatesProject = true + override val writesFiles = true override val pathArgs = listOf("file_path") override suspend fun execute(args: Map): ToolResult { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index 36b95bd5..9437aad6 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -1152,6 +1152,9 @@ class ChatViewModel( changesPaths = { call -> tools.router.getHandler(call.name)?.mutatesProject == true }, + writesFiles = { call -> + tools.router.getHandler(call.name)?.writesFiles == true + }, canonicalName = { name -> tools.router.getHandler(name)?.toolName ?: name }, requiredTool = requiredTool, // Only a prose turn that showed an answer has one a recap could repeat. diff --git a/plugins/AI-Core/src/main/res/values/strings.xml b/plugins/AI-Core/src/main/res/values/strings.xml index 4e625fce..9a33dca3 100644 --- a/plugins/AI-Core/src/main/res/values/strings.xml +++ b/plugins/AI-Core/src/main/res/values/strings.xml @@ -219,7 +219,7 @@ Reached the %1$d-step limit. Ask me to continue if the task isn\'t finished. Stopped: the model kept requesting the same action. Try rephrasing your request. - The change was not made: the file edit failed and the assistant did not manage to redo it, whatever its last message says. Try again, or ask for a smaller change. + A change was not made: a file edit failed and the assistant did not manage to redo it, whatever its last message says. Try again, or ask for a smaller change. Stopped: the model kept redoing actions it had already taken instead of making progress. Try rephrasing your request, or asking for a smaller step. Earlier messages were summarized to stay within the model\'s limit. Earlier messages could not be summarized, so only the most recent ones were kept to stay within the model\'s limit. diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index df255e82..8a146ab2 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -959,7 +959,10 @@ class AgentLoopTest { unappliedWriteTurn = { UNAPPLIED }, ) - /** Runs [writingLoop] with `edit_file` as the one tool that changes files; results come from [outcomes]. */ + /** + * Runs [writingLoop] with `edit_file` as the one tool that writes files and `run_shell_command` as one + * that changes the project without writing a file; results come from [outcomes]. + */ private suspend fun runWriting( replies: List, outcomes: Map>, @@ -978,7 +981,9 @@ class AgentLoopTest { outcomes.getValue(call.name).let { it.getOrElse(n) { _ -> it.last() } } } }, - changesPaths = { it.name == "edit_file" }, + pathsOf = { call -> setOfNotNull(call.args["path"]?.toString()) }, + changesPaths = { it.name == "edit_file" || it.name == "run_shell_command" }, + writesFiles = { it.name == "edit_file" }, events = object : AgentLoop.Events { override suspend fun onUnappliedWriteAnswer(turn: Int) { events += "unapplied" } override suspend fun onFinalAnswer(turn: Int, message: String) { events += "final" } @@ -1050,9 +1055,9 @@ class AgentLoopTest { } @Test - fun givenAFailedEditFollowedByASuccessfulOne_whenTheModelAnswers_thenNothingIsOutstanding() = runTest { + fun givenAFailedEditFollowedByASuccessfulOneOnTheSameFile_whenTheModelAnswers_thenNothingIsOutstanding() = runTest { val (result, _, events) = runWriting( - listOf(toolCall("edit_file", "a"), toolCall("edit_file", "b"), respond), + listOf(toolCall("edit_file", "a"), toolCall("edit_file", "a"), respond), mapOf("edit_file" to listOf(failedEdit, appliedEdit)), ) @@ -1060,6 +1065,74 @@ class AgentLoopTest { assertEquals(listOf("final"), events) } + @Test + fun givenAFailedEditAndASuccessfulOneOnAnotherFileInOneBatch_whenTheModelAnswers_thenItIsToldTheChangeWasNotMade() = runTest { + // One run-wide flag let the second file's success hide the first file's failure. + val (result, _, events) = runWriting( + listOf(toolCall("edit_file", "a") + toolCall("edit_file", "b"), respond, respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAWarnedFailureThatWasRedone_whenALaterEditFailsAndTheModelAnswers_thenItIsWarnedAgain() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file", "a"), respond, toolCall("edit_file", "a"), toolCall("edit_file", "b"), respond, respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit, failedEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "unapplied", "not-made"), events) + } + + @Test + fun givenAFailedEdit_whenTheModelAnswersInProse_thenItIsToldTheChangeWasNotMade() = runTest { + val (result, history, events) = runWriting( + listOf(toolCall("edit_file", "a"), toolCall("read_file", "a"), "I renamed the app.", "I renamed the app."), + mapOf("edit_file" to listOf(failedEdit), "read_file" to listOf(ToolResult.success("lines"))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + assertEquals(1, history.count { it.content == UNAPPLIED }) + } + + @Test + fun givenAFailedShellCommand_whenTheModelAnswers_thenItIsNotToldAFileChangeFailed() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("run_shell_command"), respond), + mapOf("run_shell_command" to listOf(ToolResult.failure("command did not start"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedEditThenADeniedRetry_whenTheModelAnswers_thenItIsNotSentBackToTheChangeTheUserDenied() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file", "a"), toolCall("edit_file", "a"), respond), + mapOf("edit_file" to listOf(failedEdit, ToolResult.denied("User denied permission"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedEdit_whenTheModelOnlyReadsTheFileThroughTheShell_thenItIsStillToldTheChangeWasNotMade() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file", "a"), toolCall("run_shell_command"), respond, respond), + mapOf("edit_file" to listOf(failedEdit), "run_shell_command" to listOf(ToolResult.success(""))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + private companion object { const val UNFINISHED = "finish or carry on" const val UNAPPLIED = "the change was not made" From 6a930522445df03ae5c63764d96e9d0dd09eade1 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 16:11:09 -0700 Subject: [PATCH 05/11] ADFA-6835 | Key a failed change by its file, and tell the user however the run ends An independent review of #157 at aaecc13 found that the per-path tracking could still report a change that was made as not made, or let a half-made one pass: - A failure was keyed by the path as typed, so a retry under the path edit_file corrected it to never cleared it. Writes are now keyed by the file (PathGuard.writeKeyOf): its canonical path, or its bare name when the path named no file. - A correction or an approval timeout counted as a failed change, so the run was sent back to a change the user had steered away from. These now return ToolResult.notApproved, settled like a denial. - A success cleared every failure on its file, even one beside it in the same batch. A success now settles only failures from earlier batches; the same call twice in one batch is not counted as failed. A success in a later batch is still taken for the redo, since which change a retry redoes cannot be read off its text. - After the warning, prose such as "I will now redo it" ended the run; it now gets the unfinished-turn nudge first. - A repeated look ended the run without the warning, and ignored a null unappliedWriteTurn. It now warns first, and does nothing when the check is off. - Runs that stopped as UNPARSABLE, CYCLING, REPEATED or MAX_ITERATIONS never told the user. One UnappliedWrites.warn/stop pair now serves every ending. The closest-lines hint now scores whole lines and marks one cut at 200 characters, shows a tab as \t, matches words in any script, and scores at most 32 words of old_string. 17 of the 18 new tests fail with the old behaviour restored; the other guards against matching two files that share a name. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- .../plugins/aicore/models/ToolResult.kt | 10 ++ .../aicore/prompt/ToolResultsPrompt.kt | 2 +- .../aicore/prompt/config/AgentPromptConfig.kt | 4 +- .../plugins/aicore/tool/AgentLoop.kt | 137 +++++++------- .../plugins/aicore/tool/Executor.kt | 2 +- .../plugins/aicore/tool/handlers/PathGuard.kt | 14 ++ .../tool/handlers/edit/NearMatchLines.kt | 39 ++-- .../plugins/aicore/viewmodel/ChatViewModel.kt | 7 + .../plugins/aicore/tool/AgentLoopTest.kt | 168 +++++++++++++++++- .../plugins/aicore/tool/ExecutorTest.kt | 1 + .../aicore/tool/handlers/PathGuardTest.kt | 28 +++ .../tool/handlers/edit/NearMatchLinesTest.kt | 40 ++++- 12 files changed, 363 insertions(+), 89 deletions(-) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt index c08ef45e..c9f61c81 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt @@ -10,6 +10,11 @@ data class ToolResult( val error_details: String? = null, /** The user refused the call at the approval dialog, so the tool never ran. */ val denied: Boolean = false, + /** + * The call stopped at the approval dialog, so the tool never ran: refused, corrected or left + * unanswered. True whenever [denied] is; only [denied] offers the user a Retry. + */ + val notApproved: Boolean = denied, ) { companion object { fun success(message: String, data: String? = null): ToolResult { @@ -23,6 +28,11 @@ data class ToolResult( fun denied(message: String): ToolResult { return ToolResult(false, message, denied = true) } + + /** A call the user corrected, or did not answer in time, at the approval dialog. */ + fun notApproved(message: String): ToolResult { + return ToolResult(false, message, notApproved = true) + } } fun toResultMap(): Map { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt index fea3a0cf..fb0ebabe 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt @@ -31,7 +31,7 @@ class ToolResultsPrompt( /** The turn sent after a reply that ran tools and ended without the terminal tool. */ suspend fun unfinished(): String = renderUnfinished(config.config(), terminalTool) - /** The turn sent after a reply that tried to finish while its last file change had failed. */ + /** The turn sent after a reply that tried to finish while a failed file change was not redone. */ suspend fun unappliedWrite(): String = renderUnappliedWrite(config.config(), terminalTool) /** The turn sent after a reply that answered before calling [tool], which the run had to call first. */ diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt index 0780eea8..c3784850 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/config/AgentPromptConfig.kt @@ -111,8 +111,8 @@ data class AgentPromptConfig( * @property afterFailure what to do next otherwise. * @property unfinished the turn after a reply that ran tools and then ended without the * terminal tool. - * @property unappliedWrite the turn after a reply that tried to finish while the last attempt to - * change a file had failed and none had succeeded since. + * @property unappliedWrite the turn after a reply that tried to finish while a file change had + * failed and no later write to that file had succeeded. * @property requiredTool the turn after a reply that answered before calling the tool the run * had to call first, such as a web search before a code review. */ diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index fedaf3c5..011543bd 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -16,7 +16,8 @@ import com.itsaky.androidide.plugins.services.LlmInferenceService.ChatMessage.Ro * @param requiredToolTurn words the turn sent when a run told to call a tool first tries to finish * without it; receives the tool's name. Null never asks. * @param unappliedWriteTurn words the turn sent when a run answers while a file change it attempted - * failed and has not been redone; null lets that answer end the run. + * failed and has not been redone. Sent once; if the run still ends with the change outstanding, + * however it ends, the user is told it was not made. Null turns the check off. */ class AgentLoop( private val formatToolResults: ToolResultsFormatter, @@ -53,6 +54,9 @@ class AgentLoop( * before it, so a rotation runs the step budget out instead. */ const val DEFAULT_MAX_TURNS_WITHOUT_PROGRESS = 3 + + /** The key [UnappliedWrites] keeps for a failed write whose call named no file. */ + private const val UNNAMED = "" } /** @@ -170,8 +174,8 @@ class AgentLoop( suspend fun onUnappliedWriteAnswer(turn: Int) {} /** - * The run ended with a failed file change still outstanding after it had been told, so - * whatever the last reply says, the change was not made. + * The run ended with a failed file change still outstanding, so whatever the last reply + * says, the change was not made. Fires after the event for how the run ended, if it has one. * * @param turn 1-based turn index. */ @@ -223,6 +227,9 @@ class AgentLoop( * @param writesFiles whether a call writes a file's content. Only these calls' failures count as a * change not made, and only their successes as one redone: a shell command also changes the * project, but its success says nothing about a failed edit. + * @param writtenFilesOf the files a [writesFiles] call writes, each as one key however the call + * spelled its path. A key that is a bare file name, for a path that named no existing file, is + * also matched by a later write to any path ending in that name. * @param canonicalName the registered name a call's tool resolves to, e.g. `Run_Shell_Command` to * `run_shell_command`; the name itself when none does. Everything keyed by tool name reads it. * @param requiredTool a tool the run must call before it may finish, e.g. a web search before a @@ -237,6 +244,7 @@ class AgentLoop( pathsOf: (ToolCall) -> Set = { emptySet() }, changesPaths: (ToolCall) -> Boolean = { false }, writesFiles: (ToolCall) -> Boolean = { false }, + writtenFilesOf: (ToolCall) -> Set = pathsOf, canonicalName: (String) -> String = { it }, requiredTool: String? = null, events: Events = object : Events {}, @@ -274,7 +282,7 @@ class AgentLoop( val unparsed = diagnoseUnparsedReply(text) if (unparsed != null) { events.onUnparsedReply(turn, unparsed) - return Result(turn, StopReason.UNPARSABLE) + return unapplied.stop(turn, StopReason.UNPARSABLE, events) } if (requiredPending) { requiredPending = false @@ -282,21 +290,16 @@ class AgentLoop( continue } // A prose answer can claim a failed change as readily as a terminal-tool one. - if (unapplied.outstanding && unappliedWriteTurn != null) { - if (unapplied.warnOnce()) { - warnOfUnappliedWrite(turn, history, events) - continue - } - events.onChangeNotMade(turn) - return Result(turn, StopReason.ABANDONED) - } - // Asked once per stretch of prose, so a model that will not call it still stops. + if (unapplied.warn(turn, history, events)) continue + // Asked once per stretch of prose, so a model that will not call it still stops. Also + // after the warning: prose then may be the model saying how it will redo the change. if (toolsRan && !askedToFinish && unfinishedTurn != null) { askedToFinish = true events.onUnfinishedReply(turn) history.add(ChatMessage(Role.USER, unfinishedTurn.invoke())) continue } + if (unapplied.outstanding) return unapplied.stop(turn, StopReason.ABANDONED, events) // Prose after a failed batch is the model giving up, not finishing: the run ends // with the user's request unmet, so reporting it COMPLETED overstates the outcome. if (progress.lastBatchFailed) { @@ -319,15 +322,9 @@ class AgentLoop( } val terminal = terminalTool?.let { tt -> calls.firstOrNull { isTerminalToolName(it.name, tt) } } if (terminal != null && realCalls.isEmpty()) { - if (unapplied.outstanding && unappliedWriteTurn != null) { - if (unapplied.warnOnce()) { - warnOfUnappliedWrite(turn, history, events) - continue - } - // Told once and answered again: the user's change is still not made. - events.onChangeNotMade(turn) - return Result(turn, StopReason.ABANDONED) - } + if (unapplied.warn(turn, history, events)) continue + // Told once and answered again: the user's change is still not made. + if (unapplied.outstanding) return unapplied.stop(turn, StopReason.ABANDONED, events) events.onFinalAnswer(turn, respondMessageOf(terminal.args).orEmpty()) return Result(turn, StopReason.COMPLETED) } @@ -335,20 +332,18 @@ class AgentLoop( when (progress.inspect(realCalls)) { ToolCallProgressGuard.Verdict.CYCLING -> { events.onNoProgressCycle(turn, progress.staleTurns) - return Result(turn, StopReason.CYCLING) + return unapplied.stop(turn, StopReason.CYCLING, events) } ToolCallProgressGuard.Verdict.ASSUME_COMPLETE -> { // A repeated look is not completion while the change the user asked for failed. - if (unapplied.outstanding) { - events.onChangeNotMade(turn) - return Result(turn, StopReason.ABANDONED) - } + if (unapplied.warn(turn, history, events)) continue + if (unapplied.outstanding) return unapplied.stop(turn, StopReason.ABANDONED, events) events.onRepeatAfterSuccess(turn) return Result(turn, StopReason.COMPLETED) } ToolCallProgressGuard.Verdict.REPEATED -> { events.onRepeatedToolCalls(turn) - return Result(turn, StopReason.REPEATED) + return unapplied.stop(turn, StopReason.REPEATED, events) } ToolCallProgressGuard.Verdict.PROCEED -> Unit } @@ -358,62 +353,76 @@ class AgentLoop( toolsRan = true askedToFinish = false progress.recordResults(results) - realCalls.zip(results).filter { (call, _) -> writesFiles(call) }.forEach { (call, result) -> - unapplied.record(pathsOf(call), result) - } + unapplied.record(realCalls.zip(results).filter { (call, _) -> writesFiles(call) }, writtenFilesOf) events.onToolResults(turn, realCalls, results) history.add(ChatMessage(Role.USER, formatToolResults.format(realCalls, results))) } events.onMaxIterationsReached(turn) - return Result(turn, StopReason.MAX_ITERATIONS) - } - - /** Records the answer as not finishing the run and tells it a file change it attempted was not made. */ - private suspend fun warnOfUnappliedWrite( - turn: Int, - history: MutableList, - events: Events, - ) { - events.onUnappliedWriteAnswer(turn) - history.add(ChatMessage(Role.USER, unappliedWriteTurn!!.invoke())) + return unapplied.stop(turn, StopReason.MAX_ITERATIONS, events) } /** - * The file changes a run attempted that failed and have not been redone, keyed by the paths each - * call names, so a success on one file does not hide a failure on another. A call that names no - * path cannot be matched to a file, so any later successful write clears it. + * The file changes a run attempted that failed and have not been redone, keyed by file so a + * success on one file does not hide a failure on another, and what an answer or an ending does + * while one is outstanding. Inert when [unappliedWriteTurn] is null. */ - private class UnappliedWrites { - private val paths = mutableSetOf() + private inner class UnappliedWrites { + private val failed = mutableSetOf() private var warned = false - val outstanding: Boolean get() = paths.isNotEmpty() + /** Whether a failed change is outstanding, with the check on. */ + val outstanding: Boolean get() = unappliedWriteTurn != null && failed.isNotEmpty() - /** A denial drops the call's paths: the user has decided that change should not be made. */ - fun record(named: Set, result: ToolResult) { - val keys = named.ifEmpty { setOf(UNNAMED) } - when { - result.success -> { - paths.removeAll(keys) - paths.remove(UNNAMED) - } - result.denied -> paths.removeAll(keys) - else -> paths.addAll(keys) + /** + * Records one batch's file writes. + * + * A success, or a call stopped at the approval dialog, settles failures on its file from + * earlier batches only: the model wrote the batch before it saw any of its results, so a + * success beside a failure on the same file is another change, not the failed one redone. + * A later success on that file is taken for the redo. Which change a retry redoes cannot be + * read off its text, and taking it for another change would report a change that was made + * as not made. A call that names no file cannot be matched, so any later success settles it. + * + * @param writes the batch's file-writing calls with their results. + * @param filesOf the keys of the files a call writes. + */ + fun record(writes: List>, filesOf: (ToolCall) -> Set) { + for ((call, result) in writes) { + if (!result.success && !result.notApproved) continue + val files = filesOf(call) + // A resolved key also settles a failure that was keyed by the file's bare name. + files.forEach { failed.remove(it); failed.remove(it.substringAfterLast('/')) } + if (files.isEmpty() || result.success) failed.remove(UNNAMED) + } + // The same call twice in one batch: the second fails because the first made the change. + val succeeded = writes.filter { (_, result) -> result.success }.map { (call, _) -> call } + for ((call, result) in writes) { + if (result.success || result.notApproved || call in succeeded) continue + failed.addAll(filesOf(call).ifEmpty { setOf(UNNAMED) }) } // Re-armed once nothing is outstanding, so a later failure gets its own warning. - if (paths.isEmpty()) warned = false + if (failed.isEmpty()) warned = false } - /** True the first time a run answers over [outstanding] changes since it was last re-armed. */ - fun warnOnce(): Boolean { - if (warned) return false + /** + * Tells a run that answers over an [outstanding] failure that the change was not made; once, + * until it is re-armed. + * @return true when the run was told and goes on; false when nothing is outstanding or the + * run was told already. + */ + suspend fun warn(turn: Int, history: MutableList, events: Events): Boolean { + if (!outstanding || warned) return false warned = true + events.onUnappliedWriteAnswer(turn) + history.add(ChatMessage(Role.USER, unappliedWriteTurn!!.invoke())) return true } - private companion object { - const val UNNAMED = "" + /** Ends the run for [reason], first telling the user when a failed change is [outstanding]. */ + suspend fun stop(turn: Int, reason: StopReason, events: Events): Result { + if (outstanding) events.onChangeNotMade(turn) + return Result(turn, reason) } } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt index 6112ee82..61caa934 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt @@ -217,7 +217,7 @@ class Executor( Log.i(TAG, "($executionMode): Tool '$toolName' denied. $message") AgentTrace.stage("APPROVAL", "$toolName approved=false waitedMs=$approvalMs", AgentTrace.preview(message)) // Only an explicit Deny offers Retry; a correction steers the run that is still going. - return if (approvalResponse.deniedByUser) ToolResult.denied(message) else ToolResult.failure(message) + return if (approvalResponse.deniedByUser) ToolResult.denied(message) else ToolResult.notApproved(message) } AgentTrace.stage("APPROVAL", "$toolName approved=true waitedMs=$approvalMs") diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt index bb3fec02..9ade44de 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt @@ -203,6 +203,20 @@ object PathGuard { return PathSuggestions(relative(exact), relative(byStem)) } + /** + * One key for the file a write names, however the call spelled it, so a failed write can be + * matched to the one that redoes it: `MainActivity.kt`, `./app/src/.../MainActivity.kt` and the + * absolute path all give the same key. + * @param path a path a write call named. + * @return the canonical path of the existing file [path] names, or of the one file an edit + * would be corrected to (see [suggestPathsFor]); otherwise the bare file name. + */ + fun writeKeyOf(path: String): String { + val named = resolveWithin(path)?.takeIf { it.isFile } + ?: suggestPathsFor(path).unambiguous?.let(::resolveWithin)?.takeIf { it.isFile } + return named?.canonicalPath ?: baseNameOf(path.trim()) + } + /** * Walks the project for files matching [predicate], skipping generated/hidden trees and * refusing to descend symlinks (which could otherwise walk outside the root). diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt index d000cb0c..fd135e4a 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt @@ -8,13 +8,23 @@ package com.itsaky.androidide.plugins.aicore.tool.handlers.edit */ object NearMatchLines { - /** One candidate line, with its 1-based number and its exact text (no line ending). */ - data class Line(val number: Int, val text: String) + /** + * One candidate line. + * @property number its 1-based line number. + * @property text its exact text with no line ending, or the start of it when [cut]. + * @property cut whether the line goes on past [text]. + */ + data class Line(val number: Int, val text: String, val cut: Boolean = false) private const val MAX_LINES = 5 private const val MAX_LINE_CHARS = 200 + + /** Words matched per snippet; a huge `old_string` would otherwise cost words × file size. */ + private const val MAX_WORDS = 32 private const val MIN_WORD_CHARS = 3 - private val WORD = Regex("[A-Za-z0-9_]{$MIN_WORD_CHARS,}") + + /** Letters of any script, with their combining marks, so a localized string still matches. */ + private val WORD = Regex("[\\p{L}\\p{M}\\p{N}_]{$MIN_WORD_CHARS,}") /** * The lines of [text] sharing the most distinct words with [snippet]. @@ -24,14 +34,15 @@ object NearMatchLines { * the snippet has no word of [MIN_WORD_CHARS] or more characters or no line shares one. */ fun find(text: String, snippet: String): List { - val words = WORD.findAll(snippet).map { it.value.lowercase() }.distinct().toList() + val words = WORD.findAll(snippet).map { it.value.lowercase() }.distinct().take(MAX_WORDS).toList() if (words.isEmpty()) return emptyList() return text.lineSequence() - .mapIndexed { index, raw -> Line(index + 1, raw.trimEnd('\r').take(MAX_LINE_CHARS)) } - .filter { it.text.isNotBlank() } - .map { line -> - val lower = line.text.lowercase() - line to words.count { lower.contains(it) } + .withIndex() + .filter { (_, line) -> line.isNotBlank() } + .map { (index, line) -> + val lower = line.lowercase() + Line(index + 1, line.take(MAX_LINE_CHARS), line.length > MAX_LINE_CHARS) to + words.count { lower.contains(it) } } .filter { (_, score) -> score > 0 } .sortedByDescending { (_, score) -> score } @@ -41,14 +52,18 @@ object NearMatchLines { } /** - * Renders [lines] for a rejection message, keeping leading and trailing spaces visible. + * Renders [lines] for a rejection message, keeping leading and trailing spaces visible and a + * tab told apart from them. * @param lines the result of [find]. * @return the text to append to a rejection, or an empty string when there are no lines. */ fun render(lines: List): String { if (lines.isEmpty()) return "" - val body = lines.joinToString("\n") { "line ${it.number}: \"${it.text}\"" } + val body = lines.joinToString("\n") { line -> + val shown = "line ${line.number}: \"${line.text.replace("\t", "\\t")}\"" + if (line.cut) "$shown (only the first $MAX_LINE_CHARS characters; the line goes on)" else shown + } return "\nClosest lines in the file (the exact text is between the outer double quotes, " + - "spaces included):\n$body" + "spaces included; a tab is shown as \\t):\n$body" } } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index 9437aad6..1c1bf09e 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -47,6 +47,7 @@ import com.itsaky.androidide.plugins.aicore.tool.ToolOutputBudget import com.itsaky.androidide.plugins.aicore.tool.pathsIn import com.itsaky.androidide.plugins.aicore.tool.sources.ToolSourceStore import com.itsaky.androidide.plugins.aicore.tool.handlers.BuiltInToolHandlers +import com.itsaky.androidide.plugins.aicore.tool.handlers.PathGuard import com.itsaky.androidide.plugins.aicore.tool.web.BackendWebSearch import com.itsaky.androidide.plugins.aicore.tool.web.VerificationPolicy import com.itsaky.androidide.plugins.aicore.tool.web.WebAccess @@ -1152,6 +1153,12 @@ class ChatViewModel( changesPaths = { call -> tools.router.getHandler(call.name)?.mutatesProject == true }, + // Keyed by the file, so a retry under the path an edit was corrected to still + // counts as the failed change redone. + writtenFilesOf = { call -> + tools.router.getHandler(call.name)?.pathsIn(call.args).orEmpty() + .mapTo(mutableSetOf(), PathGuard::writeKeyOf) + }, writesFiles = { call -> tools.router.getHandler(call.name)?.writesFiles == true }, diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index 8a146ab2..9102f626 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -952,26 +952,31 @@ class AgentLoopTest { } /** [finishingLoop], also telling a run that answers over a failed file change it was not made. */ - private fun writingLoop() = AgentLoop( + private fun writingLoop(maxIterations: Int = AgentLoop.DEFAULT_MAX_ITERATIONS) = AgentLoop( formatToolResults = ToolResultsPrompt({ shippedConfig }, "respond"), + maxIterations = maxIterations, terminalTool = "respond", unfinishedTurn = { UNFINISHED }, unappliedWriteTurn = { UNAPPLIED }, ) /** - * Runs [writingLoop] with `edit_file` as the one tool that writes files and `run_shell_command` as one - * that changes the project without writing a file; results come from [outcomes]. + * Runs [loop] with `edit_file` as the one tool that writes files and `run_shell_command` as one + * that changes the project without writing a file; results come from [outcomes]. A call's file + * is its `path` argument unless [writtenFilesOf] says otherwise. */ private suspend fun runWriting( replies: List, outcomes: Map>, + loop: AgentLoop = writingLoop(), + writtenFilesOf: ((ToolCall) -> Set)? = null, ): Triple, List> { val model = ScriptedModel(replies) val used = mutableMapOf() val events = mutableListOf() val history = mutableListOf(ChatMessage(Role.USER, "change the app name")) - val result = writingLoop().run( + val pathsOf = { call: ToolCall -> setOfNotNull(call.args["path"]?.toString()) } + val result = loop.run( history = history, generate = model::generate, executeTools = { calls -> @@ -981,18 +986,29 @@ class AgentLoopTest { outcomes.getValue(call.name).let { it.getOrElse(n) { _ -> it.last() } } } }, - pathsOf = { call -> setOfNotNull(call.args["path"]?.toString()) }, + pathsOf = pathsOf, changesPaths = { it.name == "edit_file" || it.name == "run_shell_command" }, writesFiles = { it.name == "edit_file" }, + writtenFilesOf = writtenFilesOf ?: pathsOf, events = object : AgentLoop.Events { override suspend fun onUnappliedWriteAnswer(turn: Int) { events += "unapplied" } override suspend fun onFinalAnswer(turn: Int, message: String) { events += "final" } override suspend fun onChangeNotMade(turn: Int) { events += "not-made" } + override suspend fun onUnfinishedReply(turn: Int) { events += "unfinished" } + override suspend fun onRepeatAfterSuccess(turn: Int) { events += "assumed-complete" } + override suspend fun onMaxIterationsReached(turns: Int) { events += "max" } + override suspend fun onUnparsedReply(turn: Int, reason: ToolCallExtractor.UnparsedReply) { + events += "unparsed" + } }, ) return Triple(result, history, events) } + /** An `edit_file` call on [path] replacing [old]; two on one path with different [old] are two changes. */ + private fun edit(path: String, old: String) = + """{"tool":"edit_file","args":{"path":"$path","old_string":"$old"}}""" + private val failedEdit = ToolResult.failure("old_string not found") private val appliedEdit = ToolResult.success("edited") @@ -1095,8 +1111,9 @@ class AgentLoopTest { mapOf("edit_file" to listOf(failedEdit), "read_file" to listOf(ToolResult.success("lines"))), ) + // Told, then asked to carry on, and still only talking: the change was not made. assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) - assertEquals(listOf("unapplied", "not-made"), events) + assertEquals(listOf("unapplied", "unfinished", "not-made"), events) assertEquals(1, history.count { it.content == UNAPPLIED }) } @@ -1133,6 +1150,145 @@ class AgentLoopTest { assertEquals(listOf("unapplied", "not-made"), events) } + @Test + fun givenAFailedEditRetriedUnderThePathItWasCorrectedTo_whenTheModelAnswers_thenNothingIsOutstanding() = runTest { + // "Main.kt" was corrected to app/Main.kt and failed; the retry used the path the rejection showed. + val files = mapOf("Main.kt" to "/p/app/Main.kt", "app/Main.kt" to "/p/app/Main.kt") + val (result, _, events) = runWriting( + listOf(edit("Main.kt", "guess"), edit("app/Main.kt", "real"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + writtenFilesOf = { call -> setOfNotNull(files[call.args["path"]]) }, + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedEditOnAPathNamingNoFile_whenAFileOfThatNameIsWritten_thenNothingIsOutstanding() = runTest { + // "File does not exist: src/Main.kt — did you mean ...": only the name can key that failure. + val files = mapOf("src/Main.kt" to "Main.kt", "app/src/Main.kt" to "/p/app/src/Main.kt") + val (result, _, events) = runWriting( + listOf(edit("src/Main.kt", "guess"), edit("app/src/Main.kt", "real"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + writtenFilesOf = { call -> setOfNotNull(files[call.args["path"]]) }, + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedEditOnOneFile_whenAnotherFileOfTheSameNameIsWritten_thenItIsStillOutstanding() = runTest { + val files = mapOf( + "values/strings.xml" to "/p/values/strings.xml", + "values-es/strings.xml" to "/p/values-es/strings.xml", + ) + val (result, _, events) = runWriting( + listOf(edit("values/strings.xml", "guess"), edit("values-es/strings.xml", "real"), respond, respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + writtenFilesOf = { call -> setOfNotNull(files[call.args["path"]]) }, + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAnEditTheUserCorrected_whenTheModelWritesAnotherFileAndAnswers_thenItIsNotToldTheChangeFailed() = runTest { + // A correction, like an approval left to time out, never ran the tool: the user steered it. + val (result, _, events) = runWriting( + listOf(edit("Foo.kt", "x"), edit("Bar.kt", "x"), respond), + mapOf("edit_file" to listOf(ToolResult.notApproved("put it in Bar.kt"), appliedEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedEditAndAnotherEditToTheSameFileThatAppliedInOneBatch_whenTheModelAnswers_thenItIsToldTheChangeWasNotMade() = runTest { + // A rename's import line failed and its call site applied: the success is another change, not the redo. + val (result, _, events) = runWriting( + listOf(edit("a", "import foo") + edit("a", "foo()"), respond, respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenTheSameEditTwiceInOneBatch_whenTheSecondFailsBecauseTheFirstApplied_thenNothingIsOutstanding() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "foo()") + edit("a", "foo()"), respond), + mapOf("edit_file" to listOf(appliedEdit, failedEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAWarnedFailure_whenTheModelSaysInProseHowItWillRedoIt_thenItIsAskedToCarryOnAndCanFinish() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "guess"), respond, "I will now replace the line.", edit("a", "real"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("unapplied", "unfinished", "final"), events) + } + + @Test + fun givenAFailedEditThenARepeatedLook_whenTheRunWasNotYetWarned_thenItIsWarnedAndCanStillRedoTheChange() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "guess"), toolCall("read_file", "a"), toolCall("read_file", "a"), edit("a", "real"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit), "read_file" to listOf(ToolResult.success("lines"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("unapplied", "final"), events) + } + + @Test + fun givenNoUnappliedWriteTurn_whenAFailedEditIsFollowedByARepeatedLook_thenTheRunEndsAsWithoutTheCheck() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "guess"), toolCall("read_file", "a"), toolCall("read_file", "a")), + mapOf("edit_file" to listOf(failedEdit), "read_file" to listOf(ToolResult.success("lines"))), + loop = finishingLoop(), + ) + + assertTrue(result.completed) + assertEquals(listOf("assumed-complete"), events) + } + + @Test + fun givenAFailedEdit_whenTheRunRunsOutOfSteps_thenTheUserIsToldTheChangeWasNotMade() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "guess"), toolCall("read_file", "b"), toolCall("read_file", "c")), + mapOf("edit_file" to listOf(failedEdit), "read_file" to listOf(ToolResult.success("lines"))), + loop = writingLoop(maxIterations = 3), + ) + + assertEquals(AgentLoop.StopReason.MAX_ITERATIONS, result.reason) + assertEquals(listOf("max", "not-made"), events) + } + + @Test + fun givenAFailedEdit_whenTheNextReplyCannotBeParsed_thenTheUserIsToldTheChangeWasNotMade() = runTest { + val (result, _, events) = runWriting( + listOf( + edit("a", "guess"), + """{"tool":"edit_file","args":{"new_string":""}}""", + ), + mapOf("edit_file" to listOf(failedEdit)), + ) + + assertEquals(AgentLoop.StopReason.UNPARSABLE, result.reason) + assertEquals(listOf("unparsed", "not-made"), events) + } + private companion object { const val UNFINISHED = "finish or carry on" const val UNAPPLIED = "the change was not made" diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt index 4dcc136d..8e7396eb 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt @@ -495,6 +495,7 @@ class ExecutorTest { assertFalse(result.success) assertFalse("Retry would rewind the run the correction steers", result.denied) + assertTrue("the tool never ran, so it is no failed change to redo", result.notApproved) assertTrue(result.message.contains("use a different file")) } diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt index 16a2044a..b5583ddc 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt @@ -185,6 +185,34 @@ class PathGuardTest { } } + // --- writeKeyOf() — one key per written file, however it was spelled ------ + + @Test + fun givenOneFileSpelledThreeWays_whenKeyed_thenEachGivesItsCanonicalPath() { + val file = File(projectRoot, "app/src/Main.kt").apply { parentFile!!.mkdirs(); writeText("") } + + val keys = listOf("app/src/Main.kt", "./app/src/../src/Main.kt", file.absolutePath).map(PathGuard::writeKeyOf) + + assertEquals(listOf(file.canonicalPath), keys.distinct()) + } + + @Test + fun givenTheBareNameOfOneProjectFile_whenKeyed_thenItGivesThatFilesCanonicalPath() { + // An edit of "Main.kt" is corrected to this file, so its retry under the full path must match. + val file = File(projectRoot, "app/src/Main.kt").apply { parentFile!!.mkdirs(); writeText("") } + + assertEquals(file.canonicalPath, PathGuard.writeKeyOf("Main.kt")) + } + + @Test + fun givenAPathNamingNoFileOrOneOfSeveral_whenKeyed_thenItGivesTheBareName() { + File(projectRoot, "a/strings.xml").apply { parentFile!!.mkdirs(); writeText("") } + File(projectRoot, "b/strings.xml").apply { parentFile!!.mkdirs(); writeText("") } + + assertEquals("Nope.kt", PathGuard.writeKeyOf("app/Nope.kt")) + assertEquals("strings.xml", PathGuard.writeKeyOf("strings.xml")) + } + // --- resolve() — the shared handler resolution policy -------------------- @Test diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt index 62913af1..69de958b 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -38,10 +38,44 @@ class NearMatchLinesTest { } @Test - fun givenALongLine_whenFound_thenItIsCut() { - val text = "foo " + "x".repeat(1_000) + fun givenALongLine_whenFoundAndRendered_thenItIsCutAndSaysSo() { + // Copied as old_string with a whole new line as new_string, an unmarked cut line leaves its tail behind. + val line = NearMatchLines.find("foo " + "x".repeat(1_000), "foo").single() - assertEquals(200, NearMatchLines.find(text, "foo").single().text.length) + assertEquals(200, line.text.length) + assertTrue(line.cut) + assertTrue(NearMatchLines.render(listOf(line)).contains("only the first 200 characters; the line goes on")) + } + + @Test + fun givenAWordPastWhereALineIsCut_whenFound_thenTheLineStillMatches() { + val text = "x".repeat(300) + " target\nother\n" + + assertEquals(listOf(1), NearMatchLines.find(text, "target").map { it.number }) + } + + @Test + fun givenATabIndentedLine_whenRendered_thenTheTabIsShownApartFromSpaces() { + val rendered = NearMatchLines.render(NearMatchLines.find("\tfoo()\n", "foo")) + + assertTrue(rendered.contains("line 1: \"\\tfoo()\"")) + assertTrue(rendered.contains("a tab is shown as \\t")) + } + + @Test + fun givenAGuessInANonLatinScript_whenFound_thenTheLineHoldingItComesBack() { + val text = "मेरा ऐप\n我的应用\n" + + assertEquals(1, NearMatchLines.find(text, "मेरा").single().number) + assertEquals(2, NearMatchLines.find(text, "我的应用").single().number) + } + + @Test + fun givenAGuessOfManyWords_whenFound_thenOnlyItsFirstWordsAreMatched() { + // The cap bounds the cost of a huge old_string, which is words × file size. + val snippet = (1..32).joinToString(" ") { "filler$it" } + " target" + + assertTrue(NearMatchLines.find("val target = 1\n", snippet).isEmpty()) } @Test From b869b928a9fc27e812c21534366ec2d9363b615a Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 16:23:33 -0700 Subject: [PATCH 06/11] ADFA-6835 | Key a failed write as its own tool resolves the path From a second independent review, of 6a93052: - writeKeyOf corrected a missing path to the one project file of that name for every write tool, but only edit_file does that. A failed create_file on build/Main.kt was keyed to app/src/Main.kt, so a retry that created lib/Main.kt left it outstanding: a made change reported as not made. New ToolHandler.correctsPaths, true only for edit_file; the other tools fall back to the bare name, and skip the project walk the correction costs. - A path with no file name ("app/") got the empty key that marks a call naming no file, which any later write settles. It now keeps the path. - NearMatchLines could cut a line between the two halves of a surrogate pair; it now stops one character short. - The unapplied-write trace and its event's KDoc said "answered", though a repeated look now triggers it too. Each of the four new tests fails with the old behaviour restored. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- .../plugins/aicore/tool/AgentLoop.kt | 6 ++-- .../plugins/aicore/tool/ToolHandler.kt | 8 ++++++ .../aicore/tool/handlers/EditFileHandler.kt | 1 + .../plugins/aicore/tool/handlers/PathGuard.kt | 15 ++++++---- .../tool/handlers/edit/NearMatchLines.kt | 5 ++-- .../aicore/viewmodel/AgentRunReporter.kt | 6 +++- .../plugins/aicore/viewmodel/ChatViewModel.kt | 5 ++-- .../handlers/BuiltInHandlerApprovalTest.kt | 11 ++++++++ .../aicore/tool/handlers/PathGuardTest.kt | 28 +++++++++++++++---- .../tool/handlers/edit/NearMatchLinesTest.kt | 10 +++++++ 10 files changed, 77 insertions(+), 18 deletions(-) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index 011543bd..8792591a 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -165,9 +165,9 @@ class AgentLoop( suspend fun onRequiredToolSkipped(turn: Int, tool: String) {} /** - * The run answered while a file change it attempted had failed and had not been redone, so - * it was told the change was not made rather than being ended on a reply that probably says - * it was. + * The run tried to finish, by answering or by repeating a look it had just taken, while a + * file change it attempted had failed and had not been redone, so it was told the change + * was not made rather than being ended on a reply that probably says it was. * * @param turn 1-based turn index. */ diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt index 9c4a74d9..b4524138 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt @@ -48,6 +48,14 @@ interface ToolHandler { val writesFiles: Boolean get() = false + /** + * Whether this tool takes a path naming no file for the one project file of that name, as + * `edit_file` does. A failed write is keyed the same way (see `PathGuard.writeKeyOf`), so a + * tool that writes exactly the path it was given is not matched to another file of that name. + */ + val correctsPaths: Boolean + get() = false + /** * JSON Schema for the arguments, in the shape the backend's tool definitions take. Empty means * untyped, flat string arguments, which is what the tool-call protocol supports today. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt index 8e0f9dd4..86de801d 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt @@ -50,6 +50,7 @@ class EditFileHandler( override val approvalPreview = ApprovalPreview.EDIT override val mutatesProject = true override val writesFiles = true + override val correctsPaths = true override val pathArgs = listOf(ARG_PATH) override val argAliases = mapOf( "old" to ARG_OLD, diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt index 9ade44de..86fb6c24 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt @@ -208,13 +208,18 @@ object PathGuard { * matched to the one that redoes it: `MainActivity.kt`, `./app/src/.../MainActivity.kt` and the * absolute path all give the same key. * @param path a path a write call named. - * @return the canonical path of the existing file [path] names, or of the one file an edit - * would be corrected to (see [suggestPathsFor]); otherwise the bare file name. + * @param correctsNames whether the tool takes a path naming no file for the one project file of + * that name (see [suggestPathsFor]), as `edit_file` does. + * @return the canonical path of the existing file [path] names, or with [correctsNames] of the + * one file it would be corrected to; otherwise the bare file name, or the path itself when it + * has none (`app/`). */ - fun writeKeyOf(path: String): String { + fun writeKeyOf(path: String, correctsNames: Boolean): String { val named = resolveWithin(path)?.takeIf { it.isFile } - ?: suggestPathsFor(path).unambiguous?.let(::resolveWithin)?.takeIf { it.isFile } - return named?.canonicalPath ?: baseNameOf(path.trim()) + ?: (if (correctsNames) suggestPathsFor(path).unambiguous else null) + ?.let(::resolveWithin)?.takeIf { it.isFile } + val trimmed = path.trim() + return named?.canonicalPath ?: baseNameOf(trimmed).ifEmpty { trimmed } } /** diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt index fd135e4a..53c41b93 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt @@ -41,8 +41,9 @@ object NearMatchLines { .filter { (_, line) -> line.isNotBlank() } .map { (index, line) -> val lower = line.lowercase() - Line(index + 1, line.take(MAX_LINE_CHARS), line.length > MAX_LINE_CHARS) to - words.count { lower.contains(it) } + // One character short rather than half of one: a lone surrogate is not text. + val shown = line.take(MAX_LINE_CHARS).let { if (it.last().isHighSurrogate()) it.dropLast(1) else it } + Line(index + 1, shown, shown.length < line.length) to words.count { lower.contains(it) } } .filter { (_, score) -> score > 0 } .sortedByDescending { (_, score) -> score } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt index 7232d30d..bf482356 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentRunReporter.kt @@ -90,7 +90,11 @@ internal class AgentRunReporter( } override suspend fun onUnappliedWriteAnswer(turn: Int) { - AgentTrace.refusal("LOOP", "turn=$turn", "answered with a failed file change outstanding; told it was not made") + AgentTrace.refusal( + "LOOP", + "turn=$turn", + "tried to finish with a failed file change outstanding; told it was not made", + ) } override suspend fun onChangeNotMade(turn: Int) { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index 1c1bf09e..2e218056 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -1156,8 +1156,9 @@ class ChatViewModel( // Keyed by the file, so a retry under the path an edit was corrected to still // counts as the failed change redone. writtenFilesOf = { call -> - tools.router.getHandler(call.name)?.pathsIn(call.args).orEmpty() - .mapTo(mutableSetOf(), PathGuard::writeKeyOf) + val handler = tools.router.getHandler(call.name) + handler?.pathsIn(call.args).orEmpty() + .mapTo(mutableSetOf()) { PathGuard.writeKeyOf(it, handler?.correctsPaths == true) } }, writesFiles = { call -> tools.router.getHandler(call.name)?.writesFiles == true diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/BuiltInHandlerApprovalTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/BuiltInHandlerApprovalTest.kt index a3727aac..a9eff41d 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/BuiltInHandlerApprovalTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/BuiltInHandlerApprovalTest.kt @@ -115,4 +115,15 @@ class BuiltInHandlerApprovalTest { builtIns.filter { it.mutatesProject }.mapTo(mutableSetOf()) { it.toolName }, ) } + + @Test + fun givenEveryBuiltIn_whenItIsRegistered_thenOnlyTheFileWritersCountForAChangeNotMadeAndOnlyAnEditCorrectsItsPath() { + // A failed write is keyed as its tool resolves the path; keyed as an edit, a failed create + // would be matched to another file of the same name. + assertEquals( + setOf("create_file", "update_file", "edit_file"), + builtIns.filter { it.writesFiles }.mapTo(mutableSetOf()) { it.toolName }, + ) + assertEquals(setOf("edit_file"), builtIns.filter { it.correctsPaths }.mapTo(mutableSetOf()) { it.toolName }) + } } diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt index b5583ddc..80452ed2 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt @@ -191,17 +191,27 @@ class PathGuardTest { fun givenOneFileSpelledThreeWays_whenKeyed_thenEachGivesItsCanonicalPath() { val file = File(projectRoot, "app/src/Main.kt").apply { parentFile!!.mkdirs(); writeText("") } - val keys = listOf("app/src/Main.kt", "./app/src/../src/Main.kt", file.absolutePath).map(PathGuard::writeKeyOf) + val keys = listOf("app/src/Main.kt", "./app/src/../src/Main.kt", file.absolutePath) + .flatMap { path -> listOf(true, false).map { PathGuard.writeKeyOf(path, correctsNames = it) } } assertEquals(listOf(file.canonicalPath), keys.distinct()) } @Test - fun givenTheBareNameOfOneProjectFile_whenKeyed_thenItGivesThatFilesCanonicalPath() { + fun givenTheBareNameOfOneProjectFile_whenKeyedForAnEdit_thenItGivesThatFilesCanonicalPath() { // An edit of "Main.kt" is corrected to this file, so its retry under the full path must match. val file = File(projectRoot, "app/src/Main.kt").apply { parentFile!!.mkdirs(); writeText("") } - assertEquals(file.canonicalPath, PathGuard.writeKeyOf("Main.kt")) + assertEquals(file.canonicalPath, PathGuard.writeKeyOf("Main.kt", correctsNames = true)) + } + + @Test + fun givenAMissingPathSharingTheNameOfOneProjectFile_whenKeyedForAToolThatWritesTheExactPath_thenItGivesTheBareName() { + // create_file on build/Main.kt never meant app/src/Main.kt: keyed to that file, the retry that + // creates lib/Main.kt would leave the failure outstanding and report a made change as not made. + File(projectRoot, "app/src/Main.kt").apply { parentFile!!.mkdirs(); writeText("") } + + assertEquals("Main.kt", PathGuard.writeKeyOf("build/Main.kt", correctsNames = false)) } @Test @@ -209,8 +219,16 @@ class PathGuardTest { File(projectRoot, "a/strings.xml").apply { parentFile!!.mkdirs(); writeText("") } File(projectRoot, "b/strings.xml").apply { parentFile!!.mkdirs(); writeText("") } - assertEquals("Nope.kt", PathGuard.writeKeyOf("app/Nope.kt")) - assertEquals("strings.xml", PathGuard.writeKeyOf("strings.xml")) + assertEquals("Nope.kt", PathGuard.writeKeyOf("app/Nope.kt", correctsNames = true)) + assertEquals("strings.xml", PathGuard.writeKeyOf("strings.xml", correctsNames = true)) + } + + @Test + fun givenAPathWithNoFileName_whenKeyed_thenItKeepsThePathRatherThanAnEmptyKey() { + // An empty key is the one a call naming no file gets, which any later write settles. + File(projectRoot, "app").mkdirs() + + assertEquals("app/", PathGuard.writeKeyOf("app/", correctsNames = true)) } // --- resolve() — the shared handler resolution policy -------------------- diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt index 69de958b..179f47b1 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -47,6 +47,16 @@ class NearMatchLinesTest { assertTrue(NearMatchLines.render(listOf(line)).contains("only the first 200 characters; the line goes on")) } + @Test + fun givenALongLineWithACharacterAcrossTheCut_whenFound_thenTheCharacterIsDroppedWhole() { + // U+1F600 is two chars; cutting between them would put a lone surrogate in the prompt. + val line = NearMatchLines.find("foo " + "x".repeat(195) + "😀" + "tail", "foo").single() + + assertEquals(199, line.text.length) + assertTrue(line.cut) + assertTrue(!line.text.last().isSurrogate()) + } + @Test fun givenAWordPastWhereALineIsCut_whenFound_thenTheLineStillMatches() { val text = "x".repeat(300) + " target\nother\n" From b0a09c4b4940702b718060548aa8914bef15b77c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 29 Oct 2026 17:57:31 -0700 Subject: [PATCH 07/11] ADFA-6835 | Let a warned run redo its change, and key a failed create by its path From a third independent review, of b869b92: - The warning asks the model to read the file again, but a read it had just taken counted as a repeated look and ended the run before it could redo the change. After the warning, the guard lets the last batch run once more; a second repeat still ends the run. - An approval timeout is a plain failure again. Nobody decided against the call, so a failed change it was redoing stays outstanding. A correction is still settled like a denial. - A write that already worked, issued again in a later batch, fails because it made the change. It is no longer counted as not made. - create_file and update_file key a failure by the path they were given even when the file does not exist yet, so a write to another strings.xml no longer settles a failed create of values-es/strings.xml. - The empty bare name of a key like app/ no longer settles the failure of a call that named no file. - File keys are looked up only while a failure is outstanding. - The warning says only the failed change was not made, not to redo other changes to that file, and gives each write tool its recovery. The notice says "a file change failed". The README lists the unapplied_write turn. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- plugins/AI-Core/README.md | 2 +- .../src/main/assets/prompts/agent_loop.yml | 10 ++-- .../plugins/aicore/models/ToolResult.kt | 7 +-- .../plugins/aicore/tool/AgentLoop.kt | 48 +++++++++++------ .../plugins/aicore/tool/Executor.kt | 9 +++- .../aicore/tool/ToolApprovalManager.kt | 6 ++- .../aicore/tool/ToolCallProgressGuard.kt | 8 +++ .../plugins/aicore/tool/handlers/PathGuard.kt | 17 +++--- .../AI-Core/src/main/res/values/strings.xml | 2 +- .../plugins/aicore/tool/AgentLoopTest.kt | 54 ++++++++++++++++++- .../plugins/aicore/tool/ExecutorTest.kt | 29 +++++++--- .../aicore/tool/ToolCallProgressGuardTest.kt | 14 +++++ .../aicore/tool/handlers/PathGuardTest.kt | 24 ++++++++- 13 files changed, 183 insertions(+), 47 deletions(-) diff --git a/plugins/AI-Core/README.md b/plugins/AI-Core/README.md index 66fc0748..adb01699 100644 --- a/plugins/AI-Core/README.md +++ b/plugins/AI-Core/README.md @@ -161,7 +161,7 @@ into one document: | `rules.yml` | `rules` | Priority groups, highest first; each has a `heading` (`CRITICAL`, `IMPORTANT`, `MANDATORY`, `OPTIONAL`) and its `items`. **Adding a rule is adding an item.** | | `tools.yml` | `tools`, `tool_call_format` | What introduces the tool list, and how to write a call as text (sent only under the text protocol). The list itself is the tools the run offers (`PromptToolCatalog`). | | `ide_context.yml` | `ide_context`, `session` | One line per fact the IDE can state: open files, module paths. `session` states the device's date and time on every prompt, and that the web tools are there. | -| `agent_loop.yml` | `agent_loop`, `approval` | What the agent is told after each tool batch: the `FAILED:` marker, the truncation notice, what to do next after a success or a failure, the `unfinished` turn sent once when a run that has used tools replies without `respond`, and the `required_tool` turn sent once when a run that had to search first answers without searching. `approval` is what it is told when the user denies a call, asks for a revision, or leaves the dialog unanswered. | +| `agent_loop.yml` | `agent_loop`, `approval` | What the agent is told after each tool batch: the `FAILED:` marker, the truncation notice, what to do next after a success or a failure, the `unfinished` turn sent once when a run that has used tools replies without `respond`, the `unapplied_write` turn sent once when a run tries to finish while a file change it tried failed and was not redone, and the `required_tool` turn sent once when a run that had to search first answers without searching. `approval` is what it is told when the user denies a call, asks for a revision, or leaves the dialog unanswered. | | `context_files.yml` | `context_files` | The heading over the files the user attached to a message, and the notice inside one cut to the size limit. | | `chat_title.yml` | `chat_title` | The system prompt of the one-off request that names a chat from its first message, before the agent replies. | | `web_search.yml` | `web_search` | The system prompt of the one-off request the `web_search` tool makes through the active backend. It may use `CURRENT_TIME`, so "latest" is read as of the device's date. | diff --git a/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml b/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml index d94fb628..d8a99813 100644 --- a/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml +++ b/plugins/AI-Core/src/main/assets/prompts/agent_loop.yml @@ -33,10 +33,12 @@ agent_loop: # Sent once when a run tries to finish while a file change it attempted failed and has not been # redone: a small model tends to answer as if the change had been made. unapplied_write: >- - A file change you tried failed and has not been redone, so that file is unchanged. Do not tell - the user it was done. Read the file with read_file, then repeat the change - with the text copied exactly from it; or, if you cannot, call the "{{TERMINAL_TOOL}}" tool and - say plainly that the change was not made. + A file change you tried failed and was not made. Other changes to that file may have been + made; do not redo those. Do not tell the user it was done. To redo the failed change: for + edit_file, read the file with read_file and copy the text exactly from it; if create_file said + the file already exists, use update_file; if update_file said the file does not exist, use + create_file; otherwise fix what the error said. If you cannot, call the "{{TERMINAL_TOOL}}" + tool and say plainly that the change was not made. # Sent once when a run that had to check the web first answered without searching; TOOL is the # search tool. A backend that forces the call never sends it (see VerificationPolicy). required_tool: >- diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt index c9f61c81..322ccbc2 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt @@ -11,8 +11,9 @@ data class ToolResult( /** The user refused the call at the approval dialog, so the tool never ran. */ val denied: Boolean = false, /** - * The call stopped at the approval dialog, so the tool never ran: refused, corrected or left - * unanswered. True whenever [denied] is; only [denied] offers the user a Retry. + * The user refused or corrected the call at the approval dialog, so the tool never ran. A call + * left unanswered is a plain failure, since nobody decided against it. True whenever [denied] + * is; only [denied] offers the user a Retry. */ val notApproved: Boolean = denied, ) { @@ -29,7 +30,7 @@ data class ToolResult( return ToolResult(false, message, denied = true) } - /** A call the user corrected, or did not answer in time, at the approval dialog. */ + /** A call the user corrected at the approval dialog. */ fun notApproved(message: String): ToolResult { return ToolResult(false, message, notApproved = true) } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index 8792591a..c6b1af49 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -259,7 +259,7 @@ class AgentLoop( // Once a tool has run, only the terminal tool finishes the run; see [unfinishedTurn]. var toolsRan = false var askedToFinish = false - val unapplied = UnappliedWrites() + val unapplied = UnappliedWrites(progress) // Cleared once the tool runs or has been asked for, so a model that refuses it still stops. var requiredPending = requiredTool != null && requiredToolTurn != null while (turn < maxIterations) { @@ -366,9 +366,13 @@ class AgentLoop( * The file changes a run attempted that failed and have not been redone, keyed by file so a * success on one file does not hide a failure on another, and what an answer or an ending does * while one is outstanding. Inert when [unappliedWriteTurn] is null. + * + * @param progress the run's guard, told when the run is sent back to look at the file again. */ - private inner class UnappliedWrites { + private inner class UnappliedWrites(private val progress: ToolCallProgressGuard) { private val failed = mutableSetOf() + // Every write that has worked this run: the same call again fails because it made the change. + private val applied = mutableSetOf() private var warned = false /** Whether a failed change is outstanding, with the check on. */ @@ -377,28 +381,36 @@ class AgentLoop( /** * Records one batch's file writes. * - * A success, or a call stopped at the approval dialog, settles failures on its file from - * earlier batches only: the model wrote the batch before it saw any of its results, so a - * success beside a failure on the same file is another change, not the failed one redone. - * A later success on that file is taken for the redo. Which change a retry redoes cannot be - * read off its text, and taking it for another change would report a change that was made - * as not made. A call that names no file cannot be matched, so any later success settles it. + * A success, or a call the user refused or corrected at the approval dialog, settles + * failures on its file from earlier batches only: the model wrote the batch before it saw + * any of its results, so a success beside a failure on the same file is another change, not + * the failed one redone. A later success on that file is taken for the redo. Which change a + * retry redoes cannot be read off its text, and taking it for another change would report a + * change that was made as not made. A call that names no file cannot be matched, so any + * later success settles it. A call that already worked this run, issued again, fails + * because it made the change, so it is not counted. * * @param writes the batch's file-writing calls with their results. * @param filesOf the keys of the files a call writes. */ fun record(writes: List>, filesOf: (ToolCall) -> Set) { - for ((call, result) in writes) { - if (!result.success && !result.notApproved) continue - val files = filesOf(call) - // A resolved key also settles a failure that was keyed by the file's bare name. - files.forEach { failed.remove(it); failed.remove(it.substringAfterLast('/')) } - if (files.isEmpty() || result.success) failed.remove(UNNAMED) + // Keys are looked up only while something is outstanding: an edit's can walk the project. + if (failed.isNotEmpty()) { + for ((call, result) in writes) { + if (!result.success && !result.notApproved) continue + val files = filesOf(call) + // A resolved key also settles a failure keyed by the file's bare name. A key with + // none (`app/`) must not settle the failure of a call that names no file. + files.forEach { file -> + failed.remove(file) + file.substringAfterLast('/').takeIf { it.isNotEmpty() }?.let(failed::remove) + } + if (files.isEmpty() || result.success) failed.remove(UNNAMED) + } } - // The same call twice in one batch: the second fails because the first made the change. - val succeeded = writes.filter { (_, result) -> result.success }.map { (call, _) -> call } + writes.filter { (_, result) -> result.success }.mapTo(applied) { (call, _) -> call } for ((call, result) in writes) { - if (result.success || result.notApproved || call in succeeded) continue + if (result.success || result.notApproved || call in applied) continue failed.addAll(filesOf(call).ifEmpty { setOf(UNNAMED) }) } // Re-armed once nothing is outstanding, so a later failure gets its own warning. @@ -416,6 +428,8 @@ class AgentLoop( warned = true events.onUnappliedWriteAnswer(turn) history.add(ChatMessage(Role.USER, unappliedWriteTurn!!.invoke())) + // The turn asks for a fresh read, which must not end the run as a look repeated. + progress.allowRepeatOfLastBatch() return true } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt index 61caa934..1ee5fbf2 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/Executor.kt @@ -216,8 +216,13 @@ class Executor( val message = approvalResponse.denialMessage ?: "Action denied by user." Log.i(TAG, "($executionMode): Tool '$toolName' denied. $message") AgentTrace.stage("APPROVAL", "$toolName approved=false waitedMs=$approvalMs", AgentTrace.preview(message)) - // Only an explicit Deny offers Retry; a correction steers the run that is still going. - return if (approvalResponse.deniedByUser) ToolResult.denied(message) else ToolResult.notApproved(message) + // Only an explicit Deny offers Retry; a correction steers the run that is still going. A + // timeout decided nothing, so it is a plain failure and a failed change stays outstanding. + return when { + approvalResponse.deniedByUser -> ToolResult.denied(message) + approvalResponse.corrected -> ToolResult.notApproved(message) + else -> ToolResult.failure(message) + } } AgentTrace.stage("APPROVAL", "$toolName approved=true waitedMs=$approvalMs") diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolApprovalManager.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolApprovalManager.kt index 22cace28..a2321bc6 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolApprovalManager.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolApprovalManager.kt @@ -156,7 +156,7 @@ class ToolApprovalManager( Log.d(TAG, "User requested a correction for $toolName") // Only this attempt is denied; a tool failure is the channel the loop re-feeds. val message = ApprovalPrompt.corrected(config.config(), toolName, decision.correction.orEmpty()) - ApprovalResponse(approved = false, denialMessage = message) + ApprovalResponse(approved = false, denialMessage = message, corrected = true) } ApprovalResult.DENIED -> { Log.d(TAG, "Approval denied for $toolName") @@ -218,11 +218,13 @@ class ToolApprovalManager( /** * Result of an approval request. * @property deniedByUser the user chose Deny; not set for a correction or a timeout. + * @property corrected the user asked for the call to be revised; not set for a timeout. */ data class ApprovalResponse( val approved: Boolean, val denialMessage: String? = null, - val deniedByUser: Boolean = false + val deniedByUser: Boolean = false, + val corrected: Boolean = false, ) /** diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuard.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuard.kt index 8aea96ff..5c56f7d2 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuard.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuard.kt @@ -113,6 +113,14 @@ internal class ToolCallProgressGuard( seenSignatures.keys.removeAll(invalidated.keys) } + /** + * Lets the next batch repeat the last one without it reading as the work being finished, for a + * run just told to look again. The limits on going round in circles still count it. + */ + fun allowRepeatOfLastBatch() { + previousSignature = null + } + /** * The paths a batch names, blanks dropped. * @param calls the calls to read. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt index 86fb6c24..46db40d2 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt @@ -210,14 +210,19 @@ object PathGuard { * @param path a path a write call named. * @param correctsNames whether the tool takes a path naming no file for the one project file of * that name (see [suggestPathsFor]), as `edit_file` does. - * @return the canonical path of the existing file [path] names, or with [correctsNames] of the - * one file it would be corrected to; otherwise the bare file name, or the path itself when it - * has none (`app/`). + * @return the canonical path of the project file [path] names: for a tool that writes exactly + * the path it is given, whether or not that file exists yet; with [correctsNames], of the + * existing file or of the one file it would be corrected to. Otherwise the bare file name, or + * the path itself when it has none (`app/`). */ fun writeKeyOf(path: String, correctsNames: Boolean): String { - val named = resolveWithin(path)?.takeIf { it.isFile } - ?: (if (correctsNames) suggestPathsFor(path).unambiguous else null) - ?.let(::resolveWithin)?.takeIf { it.isFile } + // Keyed by its name alone, a failed create would be settled by any other file of that name. + val named = if (correctsNames) { + resolveWithin(path)?.takeIf { it.isFile } + ?: suggestPathsFor(path).unambiguous?.let(::resolveWithin)?.takeIf { it.isFile } + } else { + resolveWithin(path) + } val trimmed = path.trim() return named?.canonicalPath ?: baseNameOf(trimmed).ifEmpty { trimmed } } diff --git a/plugins/AI-Core/src/main/res/values/strings.xml b/plugins/AI-Core/src/main/res/values/strings.xml index 9a33dca3..1df0c9a0 100644 --- a/plugins/AI-Core/src/main/res/values/strings.xml +++ b/plugins/AI-Core/src/main/res/values/strings.xml @@ -219,7 +219,7 @@ Reached the %1$d-step limit. Ask me to continue if the task isn\'t finished. Stopped: the model kept requesting the same action. Try rephrasing your request. - A change was not made: a file edit failed and the assistant did not manage to redo it, whatever its last message says. Try again, or ask for a smaller change. + A change was not made: a file change failed and the assistant did not manage to redo it, whatever its last message says. Try again, or ask for a smaller change. Stopped: the model kept redoing actions it had already taken instead of making progress. Try rephrasing your request, or asking for a smaller step. Earlier messages were summarized to stay within the model\'s limit. Earlier messages could not be summarized, so only the most recent ones were kept to stay within the model\'s limit. diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index 9102f626..25dc50cc 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -1035,12 +1035,24 @@ class AgentLoopTest { assertEquals(listOf("unapplied", "not-made"), events) } + @Test + fun givenAnAnswerOverAFailedEdit_whenTheModelRereadsTheFileItJustReadAsTold_thenItCanStillRedoTheChange() = runTest { + // The warning asks for a read; the same read the run had just taken must not end it as done. + val (result, _, events) = runWriting( + listOf(edit("a", "guess"), toolCall("read_file", "a"), respond, toolCall("read_file", "a"), edit("a", "real"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit), "read_file" to listOf(ToolResult.success("lines"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("unapplied", "final"), events) + } + @Test fun givenAnAnswerOverAFailedEdit_whenTheModelOnlyRepeatsALookAfterTheWarning_thenTheRunIsAbandonedNotAssumedComplete() = runTest { // Seen on a device: told the edit failed, the model re-read the file, and the repeat-after- // success rule then ended the run as COMPLETED with its false "changed" answer standing. val (result, _, events) = runWriting( - listOf(toolCall("edit_file"), toolCall("read_file"), respond, toolCall("read_file")), + listOf(edit("a", "guess"), toolCall("read_file", "a"), respond, toolCall("read_file", "a"), toolCall("read_file", "a")), mapOf("edit_file" to listOf(failedEdit), "read_file" to listOf(ToolResult.success("lines"))), ) @@ -1196,7 +1208,7 @@ class AgentLoopTest { @Test fun givenAnEditTheUserCorrected_whenTheModelWritesAnotherFileAndAnswers_thenItIsNotToldTheChangeFailed() = runTest { - // A correction, like an approval left to time out, never ran the tool: the user steered it. + // A correction never ran the tool: the user steered it. A timeout is a plain failure. val (result, _, events) = runWriting( listOf(edit("Foo.kt", "x"), edit("Bar.kt", "x"), respond), mapOf("edit_file" to listOf(ToolResult.notApproved("put it in Bar.kt"), appliedEdit)), @@ -1229,6 +1241,44 @@ class AgentLoopTest { assertEquals(listOf("final"), events) } + @Test + fun givenAnEditThatApplied_whenTheModelIssuesItAgainAfterALook_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + // The repeat finds no old_string because the first call replaced it. + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), toolCall("read_file", "a"), edit("a", "foo()"), respond), + mapOf("edit_file" to listOf(appliedEdit, failedEdit), "read_file" to listOf(ToolResult.success("lines"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAFailedCallNamingNoFile_whenACallOnAPathWithNoFileNameIsCorrected_thenItIsStillOutstanding() = runTest { + // `app/` has an empty bare name, which is the key of the call that named no file. + val (result, _, events) = runWriting( + listOf(toolCall("edit_file"), edit("app/", "x"), respond, respond), + mapOf("edit_file" to listOf(failedEdit, ToolResult.notApproved("revise it"))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenNothingOutstanding_whenWritesSucceed_thenTheirFilesAreNotLookedUp() = runTest { + // Keying an edit given a bare name walks the whole project. + var lookups = 0 + val (result, _, _) = runWriting( + listOf(edit("a", "x"), edit("b", "y"), respond), + mapOf("edit_file" to listOf(appliedEdit)), + writtenFilesOf = { call -> lookups++; setOf(call.args["path"].toString()) }, + ) + + assertTrue(result.completed) + assertEquals(0, lookups) + } + @Test fun givenAWarnedFailure_whenTheModelSaysInProseHowItWillRedoIt_thenItIsAskedToCarryOnAndCanFinish() = runTest { val (result, _, events) = runWriting( diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt index 8e7396eb..073db873 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ExecutorTest.kt @@ -11,6 +11,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.test.runTest import kotlinx.coroutines.withTimeout import org.junit.After import org.junit.Assert.assertEquals @@ -462,16 +463,18 @@ class ExecutorTest { assertTrue(results.single().success) } + /** A tool that asks before it runs, and succeeds when it does. */ + private val gatedHandler = object : ToolHandler { + override val toolName = "fake_gated" + override val description = "fake gated" + override val requiresApproval = true + override suspend fun execute(args: Map) = ToolResult.success("ran") + } + /** Runs one approval-gated call, answering its dialog with [decision] once it is pending. */ private fun executeAnswering(decision: ApprovalResult, correction: String? = null): ToolResult = runBlocking { - val handler = object : ToolHandler { - override val toolName = "fake_gated" - override val description = "fake gated" - override val requiresApproval = true - override suspend fun execute(args: Map) = ToolResult.success("ran") - } val approvalManager = ToolApprovalManager({ shippedConfig }) - val executor = Executor(ToolRouter(listOf(handler)), approvalManager) + val executor = Executor(ToolRouter(listOf(gatedHandler)), approvalManager) // Dispatchers.Default: the wait below must not sit on the thread it needs to progress on. val pending = async(Dispatchers.Default) { executor.execute(listOf(ToolCall("fake_gated", emptyMap()))) } withTimeout(5_000) { @@ -499,6 +502,18 @@ class ExecutorTest { assertTrue(result.message.contains("use a different file")) } + @Test + fun givenTheDialogIsLeftUnanswered_whenExecuting_thenTheResultIsAPlainFailure() = runTest { + val executor = Executor(ToolRouter(listOf(gatedHandler)), ToolApprovalManager({ shippedConfig })) + + // Virtual time, so the five-minute wait passes at once. + val result = executor.execute(listOf(ToolCall("fake_gated", emptyMap()))).single() + + assertFalse(result.success) + assertFalse("nobody decided against it, so a failed change it redoes stays outstanding", result.notApproved) + assertTrue(result.message.contains("timed out")) + } + @Test fun givenAShellCommandOutsideTheProject_whenExecuting_thenItIsRejectedBeforeApprovalOrTheTerminal() = runBlocking { val context = mockk(relaxed = true) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuardTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuardTest.kt index 261a2085..202f81af 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuardTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolCallProgressGuardTest.kt @@ -120,6 +120,20 @@ class ToolCallProgressGuardTest { assertEquals(ToolCallProgressGuard.Verdict.ASSUME_COMPLETE, guard.inspect(call("read_file"))) } + @Test + fun givenARepeatAllowed_whenTheBatchThatJustSucceededIsReissued_thenItRunsOnceThenReadsAsComplete() { + // A run told its change failed is asked to read the file again, which it may just have done. + val guard = guard() + guard.inspect(call("read_file")) + guard.recordResults(ok) + + guard.allowRepeatOfLastBatch() + + assertEquals(ToolCallProgressGuard.Verdict.PROCEED, guard.inspect(call("read_file"))) + guard.recordResults(ok) + assertEquals(ToolCallProgressGuard.Verdict.ASSUME_COMPLETE, guard.inspect(call("read_file"))) + } + @Test fun givenOneFailingBatchRepeated_whenInspected_thenItStopsAtTheRepeatLimit() { val guard = guard(repeats = 2) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt index 80452ed2..c26493ec 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt @@ -206,12 +206,32 @@ class PathGuardTest { } @Test - fun givenAMissingPathSharingTheNameOfOneProjectFile_whenKeyedForAToolThatWritesTheExactPath_thenItGivesTheBareName() { + fun givenAMissingPathSharingTheNameOfOneProjectFile_whenKeyedForAToolThatWritesTheExactPath_thenItGivesThatPath() { // create_file on build/Main.kt never meant app/src/Main.kt: keyed to that file, the retry that // creates lib/Main.kt would leave the failure outstanding and report a made change as not made. File(projectRoot, "app/src/Main.kt").apply { parentFile!!.mkdirs(); writeText("") } - assertEquals("Main.kt", PathGuard.writeKeyOf("build/Main.kt", correctsNames = false)) + assertEquals( + File(projectRoot, "build/Main.kt").canonicalPath, + PathGuard.writeKeyOf("build/Main.kt", correctsNames = false), + ) + } + + @Test + fun givenAFileNotCreatedYet_whenKeyedForAToolThatWritesTheExactPath_thenItIsNotKeyedByItsName() { + // Keyed "strings.xml", a failed create of the Spanish file was settled by any write to the default one. + File(projectRoot, "values/strings.xml").apply { parentFile!!.mkdirs(); writeText("") } + val missing = File(projectRoot, "values-es/strings.xml").canonicalPath + + val keys = listOf("values-es/strings.xml", "./values/../values-es/strings.xml") + .map { PathGuard.writeKeyOf(it, correctsNames = false) } + + assertEquals(listOf(missing), keys.distinct()) + } + + @Test + fun givenAPathOutsideTheProject_whenKeyedForAToolThatWritesTheExactPath_thenItGivesTheBareName() { + assertEquals("Main.kt", PathGuard.writeKeyOf("../elsewhere/Main.kt", correctsNames = false)) } @Test From c1a981077ff222c47feea0fbdeffaa6edfc001fa Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 30 Oct 2026 10:34:50 -0700 Subject: [PATCH 08/11] ADFA-6835 | Count a repeated edit as failed once another write changed its file, match whole words A write that already worked and fails again was taken for the change already made, even when a different write to the same file came between and may have undone it. It now counts as a failed change then. The files are looked up only for such a call. Recording writes does nothing when the unapplied-write check is off, so no file is keyed. The closest-lines hint matched substrings, so "val" scored a line holding "evaluate". It now matches whole words, as its description says. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ --- .../plugins/aicore/tool/AgentLoop.kt | 32 +++++++++++++-- .../tool/handlers/edit/NearMatchLines.kt | 6 +-- .../plugins/aicore/tool/AgentLoopTest.kt | 41 +++++++++++++++++++ .../tool/handlers/edit/NearMatchLinesTest.kt | 8 ++++ 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index c6b1af49..ccab5d79 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -371,8 +371,9 @@ class AgentLoop( */ private inner class UnappliedWrites(private val progress: ToolCallProgressGuard) { private val failed = mutableSetOf() - // Every write that has worked this run: the same call again fails because it made the change. - private val applied = mutableSetOf() + // Every write that has worked this run, in order: the same call again fails because it made + // the change, unless a write to its file came between. + private val applied = mutableListOf() private var warned = false /** Whether a failed change is outstanding, with the check on. */ @@ -388,12 +389,14 @@ class AgentLoop( * retry redoes cannot be read off its text, and taking it for another change would report a * change that was made as not made. A call that names no file cannot be matched, so any * later success settles it. A call that already worked this run, issued again, fails - * because it made the change, so it is not counted. + * because it made the change, so it is not counted; unless another write to its file + * worked in between, which may have undone it, so then the failure counts. * * @param writes the batch's file-writing calls with their results. * @param filesOf the keys of the files a call writes. */ fun record(writes: List>, filesOf: (ToolCall) -> Set) { + if (unappliedWriteTurn == null) return // Keys are looked up only while something is outstanding: an edit's can walk the project. if (failed.isNotEmpty()) { for ((call, result) in writes) { @@ -408,15 +411,36 @@ class AgentLoop( if (files.isEmpty() || result.success) failed.remove(UNNAMED) } } + val earlier = applied.size writes.filter { (_, result) -> result.success }.mapTo(applied) { (call, _) -> call } for ((call, result) in writes) { - if (result.success || result.notApproved || call in applied) continue + if (result.success || result.notApproved || madeTheChange(call, earlier, filesOf)) continue failed.addAll(filesOf(call).ifEmpty { setOf(UNNAMED) }) } // Re-armed once nothing is outstanding, so a later failure gets its own warning. if (failed.isEmpty()) warned = false } + /** + * Whether [call], which failed, already worked, so it fails because it made the change. + * Not so when a different write to its file worked after it in an earlier batch: that + * may have undone it. The files are looked up only then, and only for such a call. + * + * @param earlier how many of [applied] come from earlier batches. + */ + private fun madeTheChange(call: ToolCall, earlier: Int, filesOf: (ToolCall) -> Set): Boolean { + if (call !in applied) return false + val first = applied.subList(0, earlier).lastIndexOf(call) + if (first < 0) return true + val between = applied.subList(first + 1, earlier).filter { it != call } + if (between.isEmpty()) return true + val names = filesOf(call).withBareNames() + return between.none { other -> filesOf(other).withBareNames().any { it in names } } + } + + private fun Set.withBareNames(): Set = + flatMapTo(mutableSetOf()) { listOf(it, it.substringAfterLast('/')) } + /** * Tells a run that answers over an [outstanding] failure that the change was not made; once, * until it is re-armed. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt index 53c41b93..45b7a0d6 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt @@ -27,7 +27,7 @@ object NearMatchLines { private val WORD = Regex("[\\p{L}\\p{M}\\p{N}_]{$MIN_WORD_CHARS,}") /** - * The lines of [text] sharing the most distinct words with [snippet]. + * The lines of [text] sharing the most distinct whole words with [snippet]. * @param text the file contents the edit was matched against. * @param snippet the `old_string` that was not found or changed nothing. * @return up to [MAX_LINES] lines, best match first and file order among equals; empty when @@ -40,10 +40,10 @@ object NearMatchLines { .withIndex() .filter { (_, line) -> line.isNotBlank() } .map { (index, line) -> - val lower = line.lowercase() + val lineWords = WORD.findAll(line).mapTo(mutableSetOf()) { it.value.lowercase() } // One character short rather than half of one: a lone surrogate is not text. val shown = line.take(MAX_LINE_CHARS).let { if (it.last().isHighSurrogate()) it.dropLast(1) else it } - Line(index + 1, shown, shown.length < line.length) to words.count { lower.contains(it) } + Line(index + 1, shown, shown.length < line.length) to words.count { it in lineWords } } .filter { (_, score) -> score > 0 } .sortedByDescending { (_, score) -> score } diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index 25dc50cc..26ec1572 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -1253,6 +1253,47 @@ class AgentLoopTest { assertEquals(listOf("final"), events) } + @Test + fun givenAnEditThatApplied_whenAnotherWriteToItsFileCameAfter_thenTheRepeatFailingIsAChangeNotMade() = runTest { + // The second edit may have removed what the first put there, so the repeat does not + // fail because it made the change. + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), edit("a", "bar()"), edit("a", "foo()"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAnEditThatApplied_whenOnlyAnotherFileWasWrittenAfter_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), edit("b", "bar()"), edit("a", "foo()"), respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenTheCheckIsOff_whenAnEditFails_thenItsFileIsNotLookedUp() = runTest { + var lookups = 0 + val (result, _, _) = runWriting( + listOf(edit("a", "x"), respond), + mapOf("edit_file" to listOf(failedEdit)), + loop = AgentLoop( + formatToolResults = ToolResultsPrompt({ shippedConfig }, "respond"), + terminalTool = "respond", + ), + writtenFilesOf = { call -> lookups++; setOf(call.args["path"].toString()) }, + ) + + assertTrue(result.completed) + assertEquals(0, lookups) + } + @Test fun givenAFailedCallNamingNoFile_whenACallOnAPathWithNoFileNameIsCorrected_thenItIsStillOutstanding() = runTest { // `app/` has an empty bare name, which is the key of the call that named no file. diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt index 179f47b1..571ca3d6 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -106,4 +106,12 @@ class NearMatchLinesTest { assertTrue(rendered.contains("line 7: \"a \"")) } + + @Test + fun givenAWordInsideLongerWords_whenFound_thenOnlyAWholeWordMatches() { + // `val` sits inside `evaluate` and `substring`, which say nothing about the line. + val text = "evaluate(substring)\nval target = 1\n" + + assertEquals(listOf(2), NearMatchLines.find(text, "val").map { it.number }) + } } From 82ed8fa1a09aafce6700634b301c7791171b895c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 30 Oct 2026 18:45:30 -0700 Subject: [PATCH 09/11] ADFA-6835 | Count a repeated write that already worked as made; match parts of identifiers and unspaced scripts A repeated write that already worked was counted as failed when another write to its file came between. No write tool deletes a file, so a write that undid the change would have let the repeat succeed. The rule fired only after unrelated edits to the file, and it compared bare file names, so values/strings.xml and values-es/strings.xml counted as one file. It is removed. The closest-lines hint matched only whole words, so `name` missed `app_name` and part of a Chinese phrase missed its line. A word now also matches a part of an identifier, split at `_` and camelCase, and a word in a script written without spaces matches anywhere in the line. `val` still misses `evaluate`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAYjFcUx64N8PMRknzL --- .../plugins/aicore/tool/AgentLoop.kt | 34 ++++-------------- .../tool/handlers/edit/NearMatchLines.kt | 35 ++++++++++++++++--- .../plugins/aicore/tool/AgentLoopTest.kt | 22 +++++++----- .../tool/handlers/edit/NearMatchLinesTest.kt | 16 +++++++++ 4 files changed, 67 insertions(+), 40 deletions(-) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index ccab5d79..bce8b65a 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -371,9 +371,8 @@ class AgentLoop( */ private inner class UnappliedWrites(private val progress: ToolCallProgressGuard) { private val failed = mutableSetOf() - // Every write that has worked this run, in order: the same call again fails because it made - // the change, unless a write to its file came between. - private val applied = mutableListOf() + // Every write that has worked this run: the same call again fails because it made the change. + private val applied = mutableSetOf() private var warned = false /** Whether a failed change is outstanding, with the check on. */ @@ -389,8 +388,10 @@ class AgentLoop( * retry redoes cannot be read off its text, and taking it for another change would report a * change that was made as not made. A call that names no file cannot be matched, so any * later success settles it. A call that already worked this run, issued again, fails - * because it made the change, so it is not counted; unless another write to its file - * worked in between, which may have undone it, so then the failure counts. + * because it made the change, so it is not counted, even after another write to its file: + * no write tool deletes a file, so a write that undid the change would let the call + * succeed again, and one that changed the same text is the model's own later edit, which + * redoing the first would overwrite. * * @param writes the batch's file-writing calls with their results. * @param filesOf the keys of the files a call writes. @@ -411,36 +412,15 @@ class AgentLoop( if (files.isEmpty() || result.success) failed.remove(UNNAMED) } } - val earlier = applied.size writes.filter { (_, result) -> result.success }.mapTo(applied) { (call, _) -> call } for ((call, result) in writes) { - if (result.success || result.notApproved || madeTheChange(call, earlier, filesOf)) continue + if (result.success || result.notApproved || call in applied) continue failed.addAll(filesOf(call).ifEmpty { setOf(UNNAMED) }) } // Re-armed once nothing is outstanding, so a later failure gets its own warning. if (failed.isEmpty()) warned = false } - /** - * Whether [call], which failed, already worked, so it fails because it made the change. - * Not so when a different write to its file worked after it in an earlier batch: that - * may have undone it. The files are looked up only then, and only for such a call. - * - * @param earlier how many of [applied] come from earlier batches. - */ - private fun madeTheChange(call: ToolCall, earlier: Int, filesOf: (ToolCall) -> Set): Boolean { - if (call !in applied) return false - val first = applied.subList(0, earlier).lastIndexOf(call) - if (first < 0) return true - val between = applied.subList(first + 1, earlier).filter { it != call } - if (between.isEmpty()) return true - val names = filesOf(call).withBareNames() - return between.none { other -> filesOf(other).withBareNames().any { it in names } } - } - - private fun Set.withBareNames(): Set = - flatMapTo(mutableSetOf()) { listOf(it, it.substringAfterLast('/')) } - /** * Tells a run that answers over an [outstanding] failure that the change was not made; once, * until it is re-armed. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt index 45b7a0d6..f71c154d 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt @@ -24,26 +24,41 @@ object NearMatchLines { private const val MIN_WORD_CHARS = 3 /** Letters of any script, with their combining marks, so a localized string still matches. */ - private val WORD = Regex("[\\p{L}\\p{M}\\p{N}_]{$MIN_WORD_CHARS,}") + private val WORD = Regex("[\\p{L}\\p{M}\\p{N}_]+") + + /** Where an identifier such as `app_name` or `appName` splits into its parts. */ + private val PART_BREAK = Regex("_+|(?<=\\p{Ll})(?=\\p{Lu})|(?<=\\p{Lu})(?=\\p{Lu}\\p{Ll})") + + /** Scripts written without spaces, where a run of letters is a phrase rather than a word. */ + private val UNSPACED = setOf( + Character.UnicodeScript.HAN, Character.UnicodeScript.HIRAGANA, Character.UnicodeScript.KATAKANA, + Character.UnicodeScript.THAI, Character.UnicodeScript.LAO, Character.UnicodeScript.KHMER, + Character.UnicodeScript.MYANMAR, + ) /** - * The lines of [text] sharing the most distinct whole words with [snippet]. + * The lines of [text] sharing the most distinct words with [snippet]. A word matches a whole + * word or a part of an identifier, so `val` misses `evaluate` but `name` finds `app_name`; a + * word in a script written without spaces matches anywhere in the line, as a part of a phrase. * @param text the file contents the edit was matched against. * @param snippet the `old_string` that was not found or changed nothing. * @return up to [MAX_LINES] lines, best match first and file order among equals; empty when * the snippet has no word of [MIN_WORD_CHARS] or more characters or no line shares one. */ fun find(text: String, snippet: String): List { - val words = WORD.findAll(snippet).map { it.value.lowercase() }.distinct().take(MAX_WORDS).toList() + val words = wordsOf(snippet).distinct().take(MAX_WORDS).toList() if (words.isEmpty()) return emptyList() + val unspaced = words.filterTo(mutableSetOf(), ::isUnspaced) return text.lineSequence() .withIndex() .filter { (_, line) -> line.isNotBlank() } .map { (index, line) -> - val lineWords = WORD.findAll(line).mapTo(mutableSetOf()) { it.value.lowercase() } + val lineWords = wordsOf(line).toSet() + val lower = if (unspaced.isEmpty()) "" else line.lowercase() // One character short rather than half of one: a lone surrogate is not text. val shown = line.take(MAX_LINE_CHARS).let { if (it.last().isHighSurrogate()) it.dropLast(1) else it } - Line(index + 1, shown, shown.length < line.length) to words.count { it in lineWords } + val score = words.count { if (it in unspaced) lower.contains(it) else it in lineWords } + Line(index + 1, shown, shown.length < line.length) to score } .filter { (_, score) -> score > 0 } .sortedByDescending { (_, score) -> score } @@ -52,6 +67,16 @@ object NearMatchLines { .toList() } + /** Each word of [text] and each part of an identifier, lowercased, of [MIN_WORD_CHARS] or more characters. */ + private fun wordsOf(text: String): Sequence = + WORD.findAll(text) + .flatMap { word -> sequenceOf(word.value) + word.value.split(PART_BREAK) } + .filter { it.length >= MIN_WORD_CHARS } + .map { it.lowercase() } + + private fun isUnspaced(word: String): Boolean = + word.codePoints().anyMatch { Character.UnicodeScript.of(it) in UNSPACED } + /** * Renders [lines] for a rejection message, keeping leading and trailing spaces visible and a * tab told apart from them. diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index 26ec1572..f5762d14 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -1254,22 +1254,28 @@ class AgentLoopTest { } @Test - fun givenAnEditThatApplied_whenAnotherWriteToItsFileCameAfter_thenTheRepeatFailingIsAChangeNotMade() = runTest { - // The second edit may have removed what the first put there, so the repeat does not - // fail because it made the change. + fun givenAnEditThatApplied_whenAnotherWriteToItsFileCameAfter_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + // Had the second edit undone the first, the repeat would have succeeded. It failed, so the + // first change is in place or was changed again by the model's own second edit. val (result, _, events) = runWriting( - listOf(edit("a", "foo()"), edit("a", "bar()"), edit("a", "foo()"), respond, respond), + listOf(edit("a", "foo()"), edit("a", "bar()"), edit("a", "foo()"), respond), mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit)), ) - assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) - assertEquals(listOf("unapplied", "not-made"), events) + assertTrue(result.completed) + assertEquals(listOf("final"), events) } @Test - fun givenAnEditThatApplied_whenOnlyAnotherFileWasWrittenAfter_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + fun givenAnEditThatApplied_whenAFileOfTheSameNameElsewhereWasWrittenAfter_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + // Two files share a bare name; neither is the other, whatever the second write did. val (result, _, events) = runWriting( - listOf(edit("a", "foo()"), edit("b", "bar()"), edit("a", "foo()"), respond), + listOf( + edit("app/src/main/res/values/strings.xml", "Old"), + edit("app/src/main/res/values-es/strings.xml", "Viejo"), + edit("app/src/main/res/values/strings.xml", "Old"), + respond, + ), mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit)), ) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt index 571ca3d6..b3d759e2 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -114,4 +114,20 @@ class NearMatchLinesTest { assertEquals(listOf(2), NearMatchLines.find(text, "val").map { it.number }) } + + @Test + fun givenAPartOfAnIdentifier_whenFound_thenTheLinesHoldingItComeBackWholeIdentifierFirst() { + val text = "my_name = 1\nother = 2\nappName = 3\n" + + assertEquals(listOf(1, 3), NearMatchLines.find(text, "name").map { it.number }) + assertEquals(listOf(3, 1), NearMatchLines.find(text, "appName").map { it.number }) + } + + @Test + fun givenAPartOfAPhraseInAScriptWithoutSpaces_whenFound_thenItsLineComesBack() { + // Chinese runs a sentence together, so the guess is a part of the line's one long run. + val text = "我的应用程序\n你好\n" + + assertEquals(listOf(1), NearMatchLines.find(text, "我的应用").map { it.number }) + } } From ddbd07e6c6cd9712fe5449c387ccf3be2dac1829 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 31 Oct 2026 00:59:04 -0700 Subject: [PATCH 10/11] ADFA-6835 | Excuse a failed repeat only when nothing has written its file since it worked 82ed8fa counted any failed repeat of a write that had worked as made. A later write to the file can undo the change in a form the repeat cannot find (a whole-file update from a stale read re-indents the old text back), and the run then ended COMPLETED with the model saying "Done" over a change that is not in the file. Three ways of reading the answer off the file's text were each fooled by some edit, one way or the other, so nothing is read off the text now. - edit_file marks an old_string-not-found failure ToolResult.madeIfUnchanged: what its repeat gives once made, if nothing has written the file since. create_file marks an existing file ToolResult.alreadyMade: no write tool deletes one, so a create that worked stays made. - AgentLoop excuses a failed write only when it repeats a change that worked this run, the same change (ToolRouter.changeOf: the tool and its declared changeArgs, read through its argument aliases) to the same file (ToolResult.file, the canonical path the tool resolved), and either alreadyMade, or madeIfUnchanged with no successful write to that file since and no write naming no file or other project change, such as a shell command, since. Anything else counts, and the run is told to look. The cost: a repeat after the model itself changed the same file again is reported as not made. - The file-writing tools report ToolResult.file on success. A change is keyed once, when it worked, so a failure no longer walks the project for each earlier success. - The closest-lines hint gains tests for whole-identifier ranking, an acronym split from the word after it, and katakana. Tests: a stale rewrite between, another write to the file between, a shell command between, a write naming no file between, a create's certain case, a repeat failing for another reason, a first attempt, the same change to another file, a respelled path, each change keyed once; the handlers' flags and resolved files (disk and editor paths); ToolRouter.changeOf with aliases and ignored arguments; create_file's change key. Each of 9 mutants of the rule fails the test named for it. Probes from four independent reviews, driving the loop with the real handlers, all pass but three: a repeated insert applying twice (ADFA-6879), the repeat after the model's own later edit (by design), and a repeat retyped with other indentation, which keys as another change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAYjFcUx64N8PMRknzL --- .../plugins/aicore/models/ToolResult.kt | 32 +++- .../plugins/aicore/tool/AgentLoop.kt | 74 +++++++-- .../plugins/aicore/tool/ToolArgs.kt | 12 ++ .../plugins/aicore/tool/ToolHandler.kt | 9 ++ .../plugins/aicore/tool/ToolRouter.kt | 9 ++ .../aicore/tool/handlers/CreateFileHandler.kt | 9 +- .../aicore/tool/handlers/EditFileHandler.kt | 18 ++- .../aicore/tool/handlers/UpdateFileHandler.kt | 3 +- .../plugins/aicore/viewmodel/ChatViewModel.kt | 3 + .../plugins/aicore/tool/AgentLoopTest.kt | 144 ++++++++++++++++-- .../plugins/aicore/tool/ToolRouterTest.kt | 22 +++ .../tool/handlers/CreateFileHandlerTest.kt | 83 ++++++++++ .../tool/handlers/EditFileHandlerTest.kt | 31 ++++ .../tool/handlers/edit/NearMatchLinesTest.kt | 23 +++ 14 files changed, 437 insertions(+), 35 deletions(-) create mode 100644 plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt index 322ccbc2..f2731c43 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/models/ToolResult.kt @@ -16,10 +16,28 @@ data class ToolResult( * is; only [denied] offers the user a Retry. */ val notApproved: Boolean = denied, + /** + * A write that failed because its change is in the file whatever was written to it since: a + * create whose file exists, as no write tool deletes one. + */ + val alreadyMade: Boolean = false, + /** + * A write that failed the way its own repeat fails once its change is made: an edit whose + * old_string is not found. That shows the change made only if nothing has written the file + * since the change worked, which only the agent loop can tell; a first attempt at text that was + * never there fails the same way. + */ + val madeIfUnchanged: Boolean = false, + /** + * The canonical path of the file a write resolved to, however the call spelled it: set by the + * file-writing tools on success and on an [alreadyMade] or [madeIfUnchanged] failure, so a + * repeat is matched to the change it repeats. Null for every other result. + */ + val file: String? = null, ) { companion object { - fun success(message: String, data: String? = null): ToolResult { - return ToolResult(true, message, data) + fun success(message: String, data: String? = null, file: String? = null): ToolResult { + return ToolResult(true, message, data, file = file) } fun failure(message: String, error_details: String? = null): ToolResult { @@ -34,6 +52,16 @@ data class ToolResult( fun notApproved(message: String): ToolResult { return ToolResult(false, message, notApproved = true) } + + /** A write to [file] that failed because its change is there; see [alreadyMade]. */ + fun alreadyMade(message: String, file: String): ToolResult { + return ToolResult(false, message, alreadyMade = true, file = file) + } + + /** A write to [file] that failed as its repeat would once made; see [madeIfUnchanged]. */ + fun madeIfUnchanged(message: String, file: String): ToolResult { + return ToolResult(false, message, madeIfUnchanged = true, file = file) + } } fun toResultMap(): Map { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index bce8b65a..e28aa5a6 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -223,13 +223,17 @@ class AgentLoop( * @param executeTools runs a batch of tool calls. * @param pathsOf the project paths a call names; the progress guard counts an earlier look at * one a later call rewrote as a new action again rather than as a repeat. - * @param changesPaths whether a call rewrites what it names. + * @param changesPaths whether a call rewrites what it names. One that succeeds without writing a + * file, such as a shell command, is taken to have touched every file. * @param writesFiles whether a call writes a file's content. Only these calls' failures count as a * change not made, and only their successes as one redone: a shell command also changes the * project, but its success says nothing about a failed edit. * @param writtenFilesOf the files a [writesFiles] call writes, each as one key however the call * spelled its path. A key that is a bare file name, for a path that named no existing file, is * also matched by a later write to any path ending in that name. + * @param changeOf what a [writesFiles] call changes apart from its file, equal for two calls that + * make the same change however they spell their arguments; the call itself by default. The + * file is the one the tool reports in [ToolResult.file]. * @param canonicalName the registered name a call's tool resolves to, e.g. `Run_Shell_Command` to * `run_shell_command`; the name itself when none does. Everything keyed by tool name reads it. * @param requiredTool a tool the run must call before it may finish, e.g. a web search before a @@ -245,6 +249,7 @@ class AgentLoop( changesPaths: (ToolCall) -> Boolean = { false }, writesFiles: (ToolCall) -> Boolean = { false }, writtenFilesOf: (ToolCall) -> Set = pathsOf, + changeOf: (ToolCall) -> Any = { it }, canonicalName: (String) -> String = { it }, requiredTool: String? = null, events: Events = object : Events {}, @@ -353,7 +358,7 @@ class AgentLoop( toolsRan = true askedToFinish = false progress.recordResults(results) - unapplied.record(realCalls.zip(results).filter { (call, _) -> writesFiles(call) }, writtenFilesOf) + unapplied.record(realCalls.zip(results), writesFiles, changesPaths, writtenFilesOf, changeOf) events.onToolResults(turn, realCalls, results) history.add(ChatMessage(Role.USER, formatToolResults.format(realCalls, results))) } @@ -371,15 +376,19 @@ class AgentLoop( */ private inner class UnappliedWrites(private val progress: ToolCallProgressGuard) { private val failed = mutableSetOf() - // Every write that has worked this run: the same call again fails because it made the change. - private val applied = mutableSetOf() + // Each change that has worked this run, with its file, and the write count it left the file at. + private val applied = mutableMapOf, Int>() + // Successful writes and project changes so far, the last to each file, and the last to any file. + private var writes = 0 + private val lastWriteTo = mutableMapOf() + private var lastWriteAnywhere = 0 private var warned = false /** Whether a failed change is outstanding, with the check on. */ val outstanding: Boolean get() = unappliedWriteTurn != null && failed.isNotEmpty() /** - * Records one batch's file writes. + * Records one batch: its file writes, and the calls that changed the project otherwise. * * A success, or a call the user refused or corrected at the approval dialog, settles * failures on its file from earlier batches only: the model wrote the batch before it saw @@ -387,17 +396,34 @@ class AgentLoop( * the failed one redone. A later success on that file is taken for the redo. Which change a * retry redoes cannot be read off its text, and taking it for another change would report a * change that was made as not made. A call that names no file cannot be matched, so any - * later success settles it. A call that already worked this run, issued again, fails - * because it made the change, so it is not counted, even after another write to its file: - * no write tool deletes a file, so a write that undid the change would let the call - * succeed again, and one that changed the same text is the model's own later edit, which - * redoing the first would overwrite. + * later success settles it. * - * @param writes the batch's file-writing calls with their results. + * A failure is not counted when it repeats a change that worked this run, the same change + * to the same file as the tool resolved it ([ToolResult.file]), and failed as that repeat + * does once the change is made: [ToolResult.alreadyMade] whatever came between, or + * [ToolResult.madeIfUnchanged] when nothing has written the file since the change worked. + * Nothing is read off the file's text: after another write to it, a rewrite from a stale read + * may have put the old text back in a form the repeat cannot find, or the model may have + * changed the text again, and the two look alike. Such a repeat counts, and the run is sent to + * look. A write that names no file, and any other call that changes the project, such as a + * shell command, count as a write to every file. + * + * @param batch the batch's calls with their results, in the order they ran. + * @param writesFiles whether a call writes a file. + * @param changesProject whether a call changes the project. * @param filesOf the keys of the files a call writes. + * @param changeOf what a call changes apart from its file, compared to tell a repeat from + * another change. */ - fun record(writes: List>, filesOf: (ToolCall) -> Set) { + fun record( + batch: List>, + writesFiles: (ToolCall) -> Boolean, + changesProject: (ToolCall) -> Boolean, + filesOf: (ToolCall) -> Set, + changeOf: (ToolCall) -> Any, + ) { if (unappliedWriteTurn == null) return + val writes = batch.filter { (call, _) -> writesFiles(call) } // Keys are looked up only while something is outstanding: an edit's can walk the project. if (failed.isNotEmpty()) { for ((call, result) in writes) { @@ -412,15 +438,33 @@ class AgentLoop( if (files.isEmpty() || result.success) failed.remove(UNNAMED) } } - writes.filter { (_, result) -> result.success }.mapTo(applied) { (call, _) -> call } - for ((call, result) in writes) { - if (result.success || result.notApproved || call in applied) continue + for ((call, result) in batch) { + val write = writesFiles(call) + val file = result.file + if (result.success) { + if (write && file != null) { + lastWriteTo[file] = ++this.writes + applied[changeOf(call) to file] = this.writes + } else if (write || changesProject(call)) { + lastWriteAnywhere = ++this.writes + } + continue + } + if (!write || result.notApproved || isMadeRepeat(call, result, changeOf)) continue failed.addAll(filesOf(call).ifEmpty { setOf(UNNAMED) }) } // Re-armed once nothing is outstanding, so a later failure gets its own warning. if (failed.isEmpty()) warned = false } + /** Whether a failed [call] repeats a change that worked and failed only because it is made. */ + private fun isMadeRepeat(call: ToolCall, result: ToolResult, changeOf: (ToolCall) -> Any): Boolean { + val file = result.file ?: return false + if (!result.alreadyMade && !result.madeIfUnchanged) return false + val madeAt = applied[changeOf(call) to file] ?: return false + return result.alreadyMade || (lastWriteTo[file] == madeAt && lastWriteAnywhere < madeAt) + } + /** * Tells a run that answers over an [outstanding] failure that the change was not made; once, * until it is re-armed. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolArgs.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolArgs.kt index 8b8688a3..579bbab7 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolArgs.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolArgs.kt @@ -73,6 +73,18 @@ fun normalizeToolArgs(handler: ToolHandler, args: Map): Map): Any { + val normalized = normalizeToolArgs(this, args) + return toolName to changeArgs.associateWith { normalized[it]?.toString() } +} + /** * The project paths a call reads or writes, as the tool will see them: normalized keys first, then * [ToolHandler.pathDefaults] for a path the call left the handler to fill in. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt index b4524138..10cc10ea 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt @@ -115,6 +115,15 @@ interface ToolHandler { val pathDefaults: Map get() = emptyMap() + /** + * The argument keys that say what a file write changes, apart from its path: two calls with the + * same values make the same change. Read only for a tool whose failures can be + * [com.itsaky.androidide.plugins.aicore.models.ToolResult.alreadyMade] or + * [com.itsaky.androidide.plugins.aicore.models.ToolResult.madeIfUnchanged]. + */ + val changeArgs: List + get() = emptyList() + /** * Alternative argument names, alias → canonical. Small models reliably invent near-miss keys * ("old" for "old_string") and the grammar constrains none of them, so the choice is remapping diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouter.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouter.kt index 5eaa1f3d..2a31ffee 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouter.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouter.kt @@ -26,6 +26,15 @@ class ToolRouter( private val suggestable: List = handlers.map { it.toolName }.filter { suggestableNames == null || it in suggestableNames } + /** + * What a file write changes apart from its file, as [AgentLoop] compares repeats: the + * [ToolHandler.changeOf] of the tool the name reaches, or the call itself for a name that + * reaches none. + * @param call the call as the model emitted it. + * @return a value equal for two calls that make the same change to the same file. + */ + fun changeOf(call: ToolCall): Any = getHandler(call.name)?.changeOf(call.args) ?: call + /** * Finds the handler for a name the model emitted. * diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt index 77e63fdb..e3a0b6c8 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandler.kt @@ -25,6 +25,7 @@ class CreateFileHandler( override val mutatesProject = true override val writesFiles = true override val pathArgs = listOf("file_path") + override val changeArgs = listOf("content") override suspend fun execute(args: Map): ToolResult { var filePath = args["file_path"]?.toString()?.trim() @@ -51,6 +52,11 @@ class CreateFileHandler( if (file.exists()) { Log.w(TAG, "File already exists: ${file.absolutePath}") + // No write tool deletes a file, so a repeat of a create that worked fails here + // whatever was written to the file since: the loop takes that repeat for made. + if (file.isFile) { + return ToolResult.alreadyMade("File already exists: $filePath", file = file.canonicalPath) + } return ToolResult.failure("File already exists: $filePath") } @@ -70,7 +76,8 @@ class CreateFileHandler( ToolResult.success( message = "Created file: $filePath (${content.length} characters)", - data = file.absolutePath + data = file.absolutePath, + file = file.canonicalPath, ) } catch (e: Exception) { Log.e(TAG, "Error creating file at $filePath", e) diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt index 86de801d..a39b1d99 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandler.kt @@ -61,6 +61,7 @@ class EditFileHandler( "replace" to ARG_NEW, "content" to ARG_NEW, ) + override val changeArgs = listOf(ARG_OLD, ARG_NEW) companion object { const val TOOL_NAME = "edit_file" @@ -298,11 +299,11 @@ class EditFileHandler( "old_string not found — ${AgentTrace.preview(oldString, 80)}", ) } - return reject( - "$ARG_OLD not found in $resolvedPath — read the file first and copy the target " + - "text exactly, including indentation and line breaks" + - NearMatchLines.render(NearMatchLines.find(original, oldString)) - ) + val message = "$ARG_OLD not found in $resolvedPath — read the file first and copy the " + + "target text exactly, including indentation and line breaks" + + NearMatchLines.render(NearMatchLines.find(original, oldString)) + // What a repeat of this edit gives once it is made; the agent loop tells which it is. + return Analysis.Rejected(ToolResult.madeIfUnchanged(message, file = file.canonicalPath)) } val found = match as FileTextMatcher.Match.Found if (found.occurrences > 1 && !replaceAll) { @@ -337,6 +338,7 @@ class EditFileHandler( private fun reject(message: String): Analysis = Analysis.Rejected(ToolResult.failure(message)) + /** * Rejection for an edit whose old and new text are the same, naming the corrected pair when the * model pasted the user's instruction instead of the file's text. @@ -445,7 +447,8 @@ class EditFileHandler( ToolResult.success( message = "Edited ${ready.filePath} in the editor ($summary); $savedNote. " + "The change can be undone with Ctrl+Z.", - data = ready.filePath + data = ready.filePath, + file = ready.file.canonicalPath, ) } } @@ -466,7 +469,8 @@ class EditFileHandler( ) ToolResult.success( message = "Edited ${ready.filePath} ($summary)", - data = ready.filePath + data = ready.filePath, + file = ready.file.canonicalPath, ) } } diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt index 3cda207c..aec3f690 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt @@ -60,7 +60,8 @@ class UpdateFileHandler( ToolResult.success( message = "Updated file: $filePath (${content.length} characters)", - data = filePath + data = filePath, + file = file.canonicalPath, ) } } catch (e: Exception) { diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index 2e218056..ab5464e3 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -1160,6 +1160,9 @@ class ChatViewModel( handler?.pathsIn(call.args).orEmpty() .mapTo(mutableSetOf()) { PathGuard.writeKeyOf(it, handler?.correctsPaths == true) } }, + // Read as the tool will, so a repeat that spells an argument another way is + // the same change; the file comes from the tool's own result. + changeOf = tools.router::changeOf, writesFiles = { call -> tools.router.getHandler(call.name)?.writesFiles == true }, diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index f5762d14..590850a1 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -963,13 +963,17 @@ class AgentLoopTest { /** * Runs [loop] with `edit_file` as the one tool that writes files and `run_shell_command` as one * that changes the project without writing a file; results come from [outcomes]. A call's file - * is its `path` argument unless [writtenFilesOf] says otherwise. + * is its `path` argument unless [writtenFilesOf] says otherwise, and its change is the call + * itself unless [changeOf] says otherwise. A success, or a failure that may be a made repeat, + * reports the file [resolvedFileOf] resolves its call to, as the real file-writing tools do. */ private suspend fun runWriting( replies: List, outcomes: Map>, loop: AgentLoop = writingLoop(), writtenFilesOf: ((ToolCall) -> Set)? = null, + changeOf: (ToolCall) -> Any = { it }, + resolvedFileOf: (ToolCall) -> String? = { it.args["path"]?.toString() }, ): Triple, List> { val model = ScriptedModel(replies) val used = mutableMapOf() @@ -984,12 +988,14 @@ class AgentLoopTest { val n = used.getOrDefault(call.name, 0) used[call.name] = n + 1 outcomes.getValue(call.name).let { it.getOrElse(n) { _ -> it.last() } } + .let { if (it.success || it.alreadyMade || it.madeIfUnchanged) it.copy(file = resolvedFileOf(call)) else it } } }, pathsOf = pathsOf, changesPaths = { it.name == "edit_file" || it.name == "run_shell_command" }, writesFiles = { it.name == "edit_file" }, writtenFilesOf = writtenFilesOf ?: pathsOf, + changeOf = changeOf, events = object : AgentLoop.Events { override suspend fun onUnappliedWriteAnswer(turn: Int) { events += "unapplied" } override suspend fun onFinalAnswer(turn: Int, message: String) { events += "final" } @@ -1011,6 +1017,7 @@ class AgentLoopTest { private val failedEdit = ToolResult.failure("old_string not found") private val appliedEdit = ToolResult.success("edited") + private val madeEdit = ToolResult(false, "old_string not found", madeIfUnchanged = true) @Test fun givenAnAnswerOverAFailedEdit_whenTheModelThenEditsSuccessfully_thenItIsToldOnceAndCompletes() = runTest { @@ -1234,7 +1241,7 @@ class AgentLoopTest { fun givenTheSameEditTwiceInOneBatch_whenTheSecondFailsBecauseTheFirstApplied_thenNothingIsOutstanding() = runTest { val (result, _, events) = runWriting( listOf(edit("a", "foo()") + edit("a", "foo()"), respond), - mapOf("edit_file" to listOf(appliedEdit, failedEdit)), + mapOf("edit_file" to listOf(appliedEdit, madeEdit)), ) assertTrue(result.completed) @@ -1243,10 +1250,10 @@ class AgentLoopTest { @Test fun givenAnEditThatApplied_whenTheModelIssuesItAgainAfterALook_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { - // The repeat finds no old_string because the first call replaced it. + // The repeat finds no old_string because the first call replaced it, and finds new_string. val (result, _, events) = runWriting( listOf(edit("a", "foo()"), toolCall("read_file", "a"), edit("a", "foo()"), respond), - mapOf("edit_file" to listOf(appliedEdit, failedEdit), "read_file" to listOf(ToolResult.success("lines"))), + mapOf("edit_file" to listOf(appliedEdit, madeEdit), "read_file" to listOf(ToolResult.success("lines"))), ) assertTrue(result.completed) @@ -1254,18 +1261,137 @@ class AgentLoopTest { } @Test - fun givenAnEditThatApplied_whenAnotherWriteToItsFileCameAfter_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { - // Had the second edit undone the first, the repeat would have succeeded. It failed, so the - // first change is in place or was changed again by the model's own second edit. + fun givenAnEditThatApplied_whenAnotherWriteToItsFileCameAfter_thenTheRepeatFailingIsAChangeNotMade() = runTest { + // The second write may have put foo() back in a form the repeat cannot find, or changed what + // foo() became; the file cannot tell which, so the run is sent to look. + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), edit("a", "bar()"), edit("a", "foo()"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, madeEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAnEditThatApplied_whenAShellCommandRanAfter_thenTheRepeatFailingIsAChangeNotMade() = runTest { + // A command can write any file. + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), toolCall("run_shell_command"), edit("a", "foo()"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, madeEdit), "run_shell_command" to listOf(ToolResult.success("ok"))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAWriteThatNamedNoFile_whenARepeatFailsAfterIt_thenTheRepeatIsAChangeNotMade() = runTest { + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), edit("b", "x"), edit("a", "foo()"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, madeEdit)), + resolvedFileOf = { call -> call.args["path"]?.toString()?.takeIf { it != "b" } }, + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAnEditThatApplied_whenItsRepeatFailsForAnotherReason_thenItIsAChangeNotMade() = runTest { + // Only a look came between, but an unreadable file is not a change in place. + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), toolCall("read_file", "a"), edit("a", "foo()"), respond, respond), + mapOf( + "edit_file" to listOf(appliedEdit, ToolResult.failure("Cannot read file: a").copy(file = "a")), + "read_file" to listOf(ToolResult.success("lines")), + ), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAWriteWhoseChangeStaysWhateverCameAfter_whenItsRepeatFails_thenItIsNotAChangeNotMade() = runTest { + // A create's file, once made, is there whatever is written to it since. val (result, _, events) = runWriting( listOf(edit("a", "foo()"), edit("a", "bar()"), edit("a", "foo()"), respond), - mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit)), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, ToolResult(false, "exists", alreadyMade = true))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAnEditThatApplied_whenALaterWritePutItsOldTextBackInAnotherForm_thenTheRepeatFailingIsAChangeNotMade() = runTest { + // Hello became Hi; a rewrite from a stale read put Hello back with other indentation, so the + // repeat finds neither its old_string nor its new_string. The title still says Hello. Even + // naming its file, a failure the tool did not mark already made counts. + val (result, _, events) = runWriting( + listOf(edit("a", "Hello"), edit("a", "subtitle"), edit("a", "Hello"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit.copy(file = "a"))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAFirstEditFailing_whenTheFileHappensToShowItsNewText_thenItIsAChangeNotMade() = runTest { + // Only a repeat of a change that worked can have failed because it was made. + val (result, _, events) = runWriting( + listOf(edit("a", "guess"), respond, respond), + mapOf("edit_file" to listOf(madeEdit)), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenAnEditThatApplied_whenItsRepeatSpellsThePathAnotherWay_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + // The tool resolves both spellings to one file and reports it; the change is the rest of the call. + val files = mapOf("strings.xml" to "/p/values/strings.xml", "app/values/strings.xml" to "/p/values/strings.xml") + val (result, _, events) = runWriting( + listOf(edit("strings.xml", "Old"), edit("app/values/strings.xml", "Old"), respond), + mapOf("edit_file" to listOf(appliedEdit, madeEdit)), + changeOf = { call -> call.args - "path" }, + resolvedFileOf = { call -> files[call.args["path"]] }, ) assertTrue(result.completed) assertEquals(listOf("final"), events) } + @Test + fun givenTheSameChangeMadeToAnotherFile_whenItsRepeatFailsAsAlreadyMade_thenItIsAChangeNotMade() = runTest { + val (result, _, events) = runWriting( + listOf(edit("values/strings.xml", "Old"), edit("values-es/strings.xml", "Old"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, madeEdit)), + changeOf = { call -> call.args - "path" }, + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), events) + } + + @Test + fun givenEditsThatApplied_whenRepeatsFailAsAlreadyMade_thenEachChangeIsKeyedOnceWhenItWorked() = runTest { + // Keying a success can walk the project; keyed again at every later failure, it was once a + // walk per earlier success per failure, and its file could have changed meaning meanwhile. + var keyed = 0 + val (result, _, _) = runWriting( + listOf(edit("a", "1"), edit("b", "2"), edit("c", "3"), edit("a", "1"), edit("b", "2"), respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, appliedEdit, madeEdit, madeEdit)), + changeOf = { call -> keyed++; call }, + ) + + assertTrue(result.completed) + assertEquals(5, keyed) + } + @Test fun givenAnEditThatApplied_whenAFileOfTheSameNameElsewhereWasWrittenAfter_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { // Two files share a bare name; neither is the other, whatever the second write did. @@ -1276,7 +1402,7 @@ class AgentLoopTest { edit("app/src/main/res/values/strings.xml", "Old"), respond, ), - mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit)), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, madeEdit)), ) assertTrue(result.completed) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouterTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouterTest.kt index f30d8c11..f8591bf9 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouterTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolRouterTest.kt @@ -5,6 +5,7 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Assert.fail @@ -34,6 +35,27 @@ class ToolRouterTest { override suspend fun execute(args: Map): ToolResult = throw error } + private class NoteWriter : ToolHandler { + override val toolName = "write_note" + override val description = "writes a note" + override val writesFiles = true + override val changeArgs = listOf("text") + override val argAliases = mapOf("body" to "text") + override suspend fun execute(args: Map) = ToolResult.success("wrote") + } + + @Test + fun givenAWriteSpelledTwoWays_whenItsChangeIsKeyed_thenBothAreOneChangeAndAnUnknownToolIsItsCall() { + // What the agent loop compares a failed repeat with: name, aliases and ignored keys don't count. + val notes = ToolRouter(listOf(NoteWriter())) + val written = notes.changeOf(ToolCall("write_note", mapOf("path" to "a.md", "text" to "hi"))) + + assertEquals(written, notes.changeOf(ToolCall("Write_Note", mapOf("path" to "./a.md", "body" to "hi", "why" to 1)))) + assertNotEquals(written, notes.changeOf(ToolCall("write_note", mapOf("path" to "a.md", "text" to "bye")))) + val unknown = ToolCall("no_such_tool", mapOf("x" to 1)) + assertEquals(unknown, notes.changeOf(unknown)) + } + private val router = ToolRouter( listOf( handler("edit_file"), diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt new file mode 100644 index 00000000..607fa915 --- /dev/null +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt @@ -0,0 +1,83 @@ +package com.itsaky.androidide.plugins.aicore.tool.handlers + +import com.itsaky.androidide.plugins.PluginContext +import com.itsaky.androidide.plugins.aicore.tool.changeOf +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.io.File +import java.nio.file.Files + +/** Unit tests for [CreateFileHandler] on a file that already exists. */ +class CreateFileHandlerTest { + + private lateinit var projectRoot: File + private lateinit var handler: CreateFileHandler + + @Before + fun setup() { + projectRoot = Files.createTempDirectory("createfile-project").toFile().canonicalFile + PathGuard.setProjectRootForTesting(projectRoot.absolutePath) + val context = mockk() + every { context.logger } returns mockk(relaxed = true) + handler = CreateFileHandler(context) + } + + @After + fun tearDown() { + PathGuard.setProjectRootForTesting(null) + PathGuard.setProjectRootProvider(null) + projectRoot.deleteRecursively() + } + + private fun create(path: String, content: String) = runBlocking { + handler.execute(mapOf("file_path" to path, "content" to content)) + } + + @Test + fun givenAFileEditedSinceItWasCreated_whenCreatedAgain_thenItFailsAsAlreadyMadeAndTheFileIsUntouched() { + // No write tool deletes a file, so a failed repeat of a create that worked shows the create + // made; the edit since is the model's own, and must not be overwritten with the old content. + val file = File(projectRoot, "Notes.md").apply { writeText("# Edit\n") } + + val result = create("Notes.md", "# Note\n") + + assertFalse(result.success) + assertTrue(result.alreadyMade) + assertEquals(file.canonicalPath, result.file) + assertEquals("# Edit\n", file.readText()) + } + + @Test + fun givenTwoCreates_whenTheirChangesAreKeyed_thenTheContentDecidesAndTheSpellingDoesNot() { + val note = handler.changeOf(mapOf("file_path" to "Notes.md", "content" to "# One\n")) + + assertEquals(note, handler.changeOf(mapOf("path" to "./Notes.md", "content" to "# One\n"))) + assertNotEquals(note, handler.changeOf(mapOf("file_path" to "Notes.md", "content" to "# Two\n"))) + } + + @Test + fun givenADirectoryAtThePath_whenCreated_thenItFailsAndIsNotTakenForMade() { + File(projectRoot, "notes").mkdirs() + + val result = create("notes", "# Notes\n") + + assertFalse(result.success) + assertFalse(result.alreadyMade) + } + + @Test + fun givenNoFile_whenCreated_thenTheResultNamesTheFileItResolvedTo() { + val result = create("./docs/Notes.md", "# Notes\n") + + assertTrue(result.success) + assertEquals(File(projectRoot, "docs/Notes.md").canonicalPath, result.file) + } +} diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt index e669058b..a1e5b3ff 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.plugins.aicore.tool.handlers import com.itsaky.androidide.plugins.PluginContext import com.itsaky.androidide.plugins.ServiceRegistry import com.itsaky.androidide.plugins.aicore.tool.Validation +import com.itsaky.androidide.plugins.aicore.tool.changeOf import com.itsaky.androidide.plugins.services.IdeEditorService import com.itsaky.androidide.plugins.services.SelectionRange import io.mockk.coEvery @@ -16,6 +17,7 @@ import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -157,6 +159,34 @@ class EditFileHandlerTest { assertFalse(result.success) assertTrue(result.message.contains("not found")) assertEquals(original, file.readText()) + // How a repeat of a made edit fails too; only the agent loop can tell the two apart. + assertTrue(result.madeIfUnchanged) + assertFalse(result.alreadyMade) + assertEquals(file.canonicalPath, result.file) + } + + @Test + fun givenAnEdit_whenApplied_thenTheResultNamesTheFileItResolvedTo() { + val file = createFile("Main.kt", "val a = 1\n") + + val result = edit("file_path" to "./Main.kt", "old_string" to "val a = 1", "new_string" to "val a = 2") + + assertTrue(result.success) + assertEquals(file.canonicalPath, result.file) + } + + @Test + fun givenARepeatSpellingItsArgumentsAnotherWay_whenItsChangeIsKeyed_thenItIsTheSameChange() { + val original = handler.changeOf(mapOf("file_path" to "Main.kt", "old_string" to "a", "new_string" to "b")) + + assertEquals(original, handler.changeOf(mapOf("path" to "./Main.kt", "old" to "a", "new" to "b"))) + assertEquals( + original, + handler.changeOf( + mapOf("file_path" to "Main.kt", "old_string" to "a", "new_string" to "b", "replace_all" to "false", "explanation" to "x"), + ), + ) + assertNotEquals(original, handler.changeOf(mapOf("file_path" to "Main.kt", "old_string" to "a", "new_string" to "c"))) } @Test @@ -482,6 +512,7 @@ class EditFileHandlerTest { val result = edit("file_path" to "Main.kt", "old_string" to "line1", "new_string" to "LINE1") assertTrue("Expected success, got: ${result.message}", result.success) + assertEquals(target.canonicalPath, result.file) verify { editorService.replaceRange(eq(target), capture(range), capture(replacement)) } // 0-based line/column, matching the host editor's coordinate space. assertEquals(1, range.captured.startLine) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt index b3d759e2..5f67f9fe 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -130,4 +130,27 @@ class NearMatchLinesTest { assertEquals(listOf(1), NearMatchLines.find(text, "我的应用").map { it.number }) } + + @Test + fun givenAWholeIdentifier_whenFound_thenALineHoldingItWholeComesBeforeOneHoldingOnlyAPart() { + val text = "X\nval app_name = 1\n" + + assertEquals(listOf(2, 1), NearMatchLines.find(text, "app_name").map { it.number }) + } + + @Test + fun givenAnAcronymBeforeAWord_whenFound_thenEachIsAPartOfTheIdentifier() { + val text = "class HTMLParser\nother\n" + + assertEquals(listOf(1), NearMatchLines.find(text, "parser").map { it.number }) + assertEquals(listOf(1), NearMatchLines.find(text, "html").map { it.number }) + } + + @Test + fun givenAPartOfAPhraseInKatakana_whenFound_thenItsLineComesBack() { + // Japanese runs words together as Chinese does, in its own scripts. + val text = "マイアプリケーション\nほかのもの\n" + + assertEquals(listOf(1), NearMatchLines.find(text, "アプリ").map { it.number }) + } } From 88230c5fd749132673ef28345c6e6eda9d012817 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Sat, 31 Oct 2026 01:25:23 -0700 Subject: [PATCH 11/11] ADFA-6835 | Test the repeat check with the real file tools; state its two costs A review of ddbd07e found the rule sound for every write the loop sees, and its tests short of it: - AgentLoopFileToolsTest drives the loop with the real edit, update and create tools through a ToolRouter, as ChatViewModel wires it: a stale rewrite through another spelling of the path, in the same batch, and to a file open in the editor each leave the repeat reported not made; a repeat in the same batch, or after an update to another file, is taken for made; the same create to another existing file counts. Six mutants the suite missed (update_file's file, canonical paths in the flags, the key's file half, the latest success of a redone change) now fail a test. - The stale-rewrite loop test uses the flag edit_file now returns; the handler flag tests spell their paths with ./, so a non-canonical file fails them; a redone change is keyed at its latest success. - AgentLoop's KDoc states both costs: any project change that names no file, even a read-only shell command or MCP tool, makes a later made repeat count; and a create's repeat is excused whatever was written to the file since. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Vh2DAYjFcUx64N8PMRknzL --- .../plugins/aicore/tool/AgentLoop.kt | 6 +- .../plugins/aicore/tool/ToolHandler.kt | 2 +- .../aicore/tool/AgentLoopFileToolsTest.kt | 199 ++++++++++++++++++ .../plugins/aicore/tool/AgentLoopTest.kt | 20 +- .../tool/handlers/CreateFileHandlerTest.kt | 2 +- .../tool/handlers/EditFileHandlerTest.kt | 2 +- 6 files changed, 223 insertions(+), 8 deletions(-) create mode 100644 plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopFileToolsTest.kt diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt index e28aa5a6..4412342e 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoop.kt @@ -405,8 +405,10 @@ class AgentLoop( * Nothing is read off the file's text: after another write to it, a rewrite from a stale read * may have put the old text back in a form the repeat cannot find, or the model may have * changed the text again, and the two look alike. Such a repeat counts, and the run is sent to - * look. A write that names no file, and any other call that changes the project, such as a - * shell command, count as a write to every file. + * look. A write that names no file, and any other call that changes the project, count as a + * write to every file: a shell command or an MCP tool may write any, so even one that only + * read (`cat`) makes a later made repeat count. A create's repeat is taken for made whatever + * came between: the file it made is still there, though what it holds may have changed. * * @param batch the batch's calls with their results, in the order they ran. * @param writesFiles whether a call writes a file. diff --git a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt index 10cc10ea..b93487e5 100644 --- a/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt @@ -117,7 +117,7 @@ interface ToolHandler { /** * The argument keys that say what a file write changes, apart from its path: two calls with the - * same values make the same change. Read only for a tool whose failures can be + * same values make the same change. Matters only for a tool whose failures can be * [com.itsaky.androidide.plugins.aicore.models.ToolResult.alreadyMade] or * [com.itsaky.androidide.plugins.aicore.models.ToolResult.madeIfUnchanged]. */ diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopFileToolsTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopFileToolsTest.kt new file mode 100644 index 00000000..a6b6537a --- /dev/null +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopFileToolsTest.kt @@ -0,0 +1,199 @@ +package com.itsaky.androidide.plugins.aicore.tool + +import com.itsaky.androidide.plugins.PluginContext +import com.itsaky.androidide.plugins.ServiceRegistry +import com.itsaky.androidide.plugins.aicore.tool.handlers.CreateFileHandler +import com.itsaky.androidide.plugins.aicore.tool.handlers.EditFileHandler +import com.itsaky.androidide.plugins.aicore.tool.handlers.PathGuard +import com.itsaky.androidide.plugins.aicore.tool.handlers.UpdateFileHandler +import com.itsaky.androidide.plugins.services.IdeEditorService +import com.itsaky.androidide.plugins.services.LlmInferenceService.ChatMessage +import com.itsaky.androidide.plugins.services.LlmInferenceService.ChatMessage.Role +import com.itsaky.androidide.plugins.services.SelectionRange +import io.mockk.coEvery +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import java.io.File +import java.nio.file.Files + +/** + * [AgentLoop]'s check for a change not made, driven with the real file-writing tools and keyed + * through a real [ToolRouter] as ChatViewModel wires it: a repeat of an edit that worked is taken for + * made only when nothing has written its file since, however each call spelled the path, and in the + * editor as on disk. + */ +class AgentLoopFileToolsTest { + + private lateinit var root: File + private lateinit var router: ToolRouter + // The editor's open buffers by canonical path; a file is open only once it has one. + private val buffers = mutableMapOf() + + @Before + fun setup() { + root = Files.createTempDirectory("agentloop-files").toFile().canonicalFile + PathGuard.setProjectRootForTesting(root.absolutePath) + val context = mockk() + val services = mockk() + val editor = mockk(relaxed = true) + every { editor.getFileContent(any()) } answers { buffers[firstArg().canonicalPath] } + every { editor.replaceRange(any(), any(), any()) } answers { + val path = firstArg().canonicalPath + val range = secondArg() + val text = buffers.getValue(path) + fun offset(line: Int, column: Int): Int { + var at = 0 + repeat(line) { at = text.indexOf('\n', at) + 1 } + return at + column + } + buffers[path] = text.substring(0, offset(range.startLine, range.startColumn)) + thirdArg() + + text.substring(offset(range.endLine, range.endColumn)) + true + } + coEvery { editor.saveFile(any()) } answers { + val file = firstArg() + file.writeText(buffers.getValue(file.canonicalPath)) + true + } + every { context.services } returns services + every { context.logger } returns mockk(relaxed = true) + every { services.get(IdeEditorService::class.java) } returns editor + router = ToolRouter(listOf(EditFileHandler(context, Dispatchers.Unconfined), UpdateFileHandler(context), CreateFileHandler(context))) + } + + @After + fun tearDown() { + PathGuard.setProjectRootForTesting(null) + root.deleteRecursively() + } + + /** Runs one batch of [script] per turn; returns the result and the loop's notices. */ + private suspend fun drive(vararg script: List): Pair> { + var turn = 0 + val notices = mutableListOf() + val loop = AgentLoop( + formatToolResults = { _, results -> results.joinToString("\n") { "${it.success}: ${it.message}" } }, + extractToolCalls = { text -> script[text.toInt()] }, + diagnoseUnparsedReply = { null }, + terminalTool = "respond", + unfinishedTurn = { "finish" }, + unappliedWriteTurn = { "UNAPPLIED" }, + ) + val result = loop.run( + history = mutableListOf(ChatMessage(Role.USER, "change the title")), + generate = { AgentLoop.ModelReply((turn++).coerceAtMost(script.size - 1).toString()) }, + executeTools = { calls -> + calls.map { call -> + val handler = router.getHandler(call.name)!! + when (val validation = handler.validate(normalizeToolArgs(handler, call.args))) { + is Validation.Accepted -> handler.execute(validation.args) + is Validation.Rejected -> validation.result + } + } + }, + pathsOf = { call -> router.getHandler(call.name)?.pathsIn(call.args).orEmpty() }, + changesPaths = { call -> router.getHandler(call.name)?.mutatesProject == true }, + writesFiles = { call -> router.getHandler(call.name)?.writesFiles == true }, + changeOf = router::changeOf, + canonicalName = { name -> router.getHandler(name)?.toolName ?: name }, + events = object : AgentLoop.Events { + override suspend fun onUnappliedWriteAnswer(turn: Int) { notices += "unapplied" } + override suspend fun onChangeNotMade(turn: Int) { notices += "not-made" } + }, + ) + return result to notices + } + + private val respond = listOf(ToolCall("respond", mapOf("message" to "Done."))) + private val original = "class A {\n val title = \"Hello\"\n}\n" + private val titleEdit = ToolCall( + "edit_file", + mapOf("file_path" to "app/A.kt", "old_string" to " val title = \"Hello\"", "new_string" to " val title = \"Hi\""), + ) + /** A whole-file rewrite from a read taken before the edit: Hello back, re-indented, so a repeat finds neither text. */ + private fun staleRewrite(path: String) = + ToolCall("update_file", mapOf("file_path" to path, "content" to "class A {\n val title = \"Hello\"\n}\n")) + + private fun file(path: String, text: String) = File(root, path).apply { parentFile!!.mkdirs(); writeText(text) } + + @Test + fun givenAStaleRewriteSpellingThePathAnotherWay_whenTheEditIsRepeated_thenTheChangeIsReportedNotMade() = runTest { + val file = file("app/A.kt", original) + + val (result, notices) = drive(listOf(titleEdit), listOf(staleRewrite("app/../app/./A.kt")), listOf(titleEdit), respond, respond) + + assertTrue(file.readText().contains("\"Hello\"")) + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), notices) + } + + @Test + fun givenAnEditInTheEditorThenARewriteOnDisk_whenTheEditIsRepeated_thenTheChangeIsReportedNotMade() = runTest { + val file = file("app/A.kt", original) + buffers[file.canonicalPath] = original + val inEditor = titleEdit.copy(args = titleEdit.args + ("file_path" to "./app/A.kt")) + + val (result, notices) = drive(listOf(inEditor), listOf(staleRewrite("app/A.kt")), listOf(inEditor), respond, respond) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), notices) + } + + @Test + fun givenAStaleRewriteInTheSameBatchAfterTheEdit_whenTheEditIsRepeated_thenTheChangeIsReportedNotMade() = runTest { + file("app/A.kt", original) + + val (result, notices) = drive(listOf(titleEdit, staleRewrite("app/A.kt")), listOf(titleEdit), respond, respond) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), notices) + } + + @Test + fun givenAnUpdateToAnotherFile_whenTheEditIsRepeatedAfterIt_thenTheChangeIsTakenForMade() = runTest { + val file = file("app/A.kt", original) + file("app/B.kt", "class B\n") + val updateOther = ToolCall("update_file", mapOf("file_path" to "./app/B.kt", "content" to "class B {}\n")) + + val (result, notices) = drive(listOf(titleEdit), listOf(updateOther), listOf(titleEdit), respond) + + assertTrue(file.readText().contains("\"Hi\"")) + assertEquals(AgentLoop.StopReason.COMPLETED, result.reason) + assertFalse("not-made" in notices) + } + + @Test + fun givenAnEditThatWorked_whenRepeatedInTheSameBatch_thenTheChangeIsTakenForMade() = runTest { + val file = file("app/A.kt", original) + + val (result, notices) = drive(listOf(titleEdit, titleEdit), respond) + + assertTrue(file.readText().contains("\"Hi\"")) + assertEquals(AgentLoop.StopReason.COMPLETED, result.reason) + assertFalse("not-made" in notices) + } + + @Test + fun givenACreateThatWorked_whenTheSameCreateOfAnotherExistingFileFails_thenItIsReportedNotMade() = runTest { + // The same change, but to another file, which already held something else. + val content = "\n" + val other = file("app/res/values-es/strings.xml", "b\n") + fun create(path: String) = ToolCall("create_file", mapOf("file_path" to path, "content" to content)) + + val (result, notices) = drive( + listOf(create("app/res/values/strings.xml")), listOf(create("app/res/values-es/strings.xml")), respond, respond, + ) + + assertFalse(other.readText() == content) + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), notices) + } +} diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt index 590850a1..f9074d96 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/AgentLoopTest.kt @@ -1273,6 +1273,21 @@ class AgentLoopTest { assertEquals(listOf("unapplied", "not-made"), events) } + @Test + fun givenAnEditRedoneAfterAnotherWriteToItsFile_whenItIsRepeatedAfterALook_thenTheRepeatIsTakenForMade() = runTest { + // The redo is the change's latest success; only writes since that one count. + val (result, _, events) = runWriting( + listOf(edit("a", "foo()"), edit("a", "bar()"), edit("a", "foo()"), toolCall("read_file", "a"), edit("a", "foo()"), respond), + mapOf( + "edit_file" to listOf(appliedEdit, appliedEdit, appliedEdit, madeEdit), + "read_file" to listOf(ToolResult.success("lines")), + ), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + @Test fun givenAnEditThatApplied_whenAShellCommandRanAfter_thenTheRepeatFailingIsAChangeNotMade() = runTest { // A command can write any file. @@ -1327,11 +1342,10 @@ class AgentLoopTest { @Test fun givenAnEditThatApplied_whenALaterWritePutItsOldTextBackInAnotherForm_thenTheRepeatFailingIsAChangeNotMade() = runTest { // Hello became Hi; a rewrite from a stale read put Hello back with other indentation, so the - // repeat finds neither its old_string nor its new_string. The title still says Hello. Even - // naming its file, a failure the tool did not mark already made counts. + // repeat finds neither its old_string nor its new_string. The title still says Hello. val (result, _, events) = runWriting( listOf(edit("a", "Hello"), edit("a", "subtitle"), edit("a", "Hello"), respond, respond), - mapOf("edit_file" to listOf(appliedEdit, appliedEdit, failedEdit.copy(file = "a"))), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, madeEdit)), ) assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt index 607fa915..60f5c095 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt @@ -47,7 +47,7 @@ class CreateFileHandlerTest { // made; the edit since is the model's own, and must not be overwritten with the old content. val file = File(projectRoot, "Notes.md").apply { writeText("# Edit\n") } - val result = create("Notes.md", "# Note\n") + val result = create("./Notes.md", "# Note\n") assertFalse(result.success) assertTrue(result.alreadyMade) diff --git a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt index a1e5b3ff..20b0777d 100644 --- a/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/EditFileHandlerTest.kt @@ -151,7 +151,7 @@ class EditFileHandlerTest { val file = createFile("Main.kt", original) val result = edit( - "file_path" to "Main.kt", + "file_path" to "./Main.kt", "old_string" to "val zzz = 9", "new_string" to "whatever", )