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 a4818c86..d8a99813 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,15 @@ 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 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 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 c08ef45e..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 @@ -10,10 +10,34 @@ 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 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, + /** + * 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 { @@ -23,6 +47,21 @@ data class ToolResult( fun denied(message: String): ToolResult { return ToolResult(false, message, denied = true) } + + /** A call the user corrected at the approval dialog. */ + 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/prompt/ToolResultsPrompt.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/prompt/ToolResultsPrompt.kt index e65fd913..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,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 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. */ 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..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,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 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. */ @@ -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..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 @@ -15,6 +15,9 @@ 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 a file change it attempted + * 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, @@ -27,6 +30,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 { @@ -50,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 = "" } /** @@ -156,6 +163,23 @@ class AgentLoop( * @param tool the tool the run had to call. */ suspend fun onRequiredToolSkipped(turn: Int, tool: String) {} + + /** + * 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. + */ + suspend fun onUnappliedWriteAnswer(turn: Int) {} + + /** + * 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. + */ + suspend fun onChangeNotMade(turn: Int) {} } /** Why the loop stopped. */ @@ -199,7 +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 @@ -213,6 +247,9 @@ class AgentLoop( executeTools: suspend (List) -> List, pathsOf: (ToolCall) -> Set = { emptySet() }, 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 {}, @@ -227,6 +264,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(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) { @@ -249,20 +287,24 @@ 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 askForRequiredTool(turn, requiredTool!!, history, events) continue } - // Asked once per stretch of prose, so a model that will not call it still stops. + // A prose answer can claim a failed change as readily as a terminal-tool one. + 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) { @@ -283,26 +325,30 @@ 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 (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) } 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.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 } @@ -312,12 +358,136 @@ class AgentLoop( toolsRan = true askedToFinish = false progress.recordResults(results) + unapplied.record(realCalls.zip(results), writesFiles, changesPaths, writtenFilesOf, changeOf) events.onToolResults(turn, realCalls, results) history.add(ChatMessage(Role.USER, formatToolResults.format(realCalls, results))) } events.onMaxIterationsReached(turn) - return Result(turn, StopReason.MAX_ITERATIONS) + return unapplied.stop(turn, StopReason.MAX_ITERATIONS, events) + } + + /** + * 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 val progress: ToolCallProgressGuard) { + private val failed = 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: 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 + * 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 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, 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. + * @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( + 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) { + 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) + } + } + 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. + * @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())) + // The turn asks for a fresh read, which must not end the run as a look repeated. + progress.allowRepeatOfLastBatch() + return true + } + + /** 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) + } } /** Records the reply as not finishing the run and asks for [tool] in the next user turn. */ 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..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.failure(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/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/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/ToolHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/ToolHandler.kt index bfa719a1..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 @@ -40,6 +40,22 @@ 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 + + /** + * 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. @@ -99,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. 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]. + */ + 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 4928dd81..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 @@ -23,7 +23,9 @@ 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 val changeArgs = listOf("content") override suspend fun execute(args: Map): ToolResult { var filePath = args["file_path"]?.toString()?.trim() @@ -50,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") } @@ -69,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 f5537013..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 @@ -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 @@ -48,6 +49,8 @@ class EditFileHandler( override val requiresApproval = true 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, @@ -58,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" @@ -236,9 +240,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 +282,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. @@ -291,10 +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" - ) + 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) { @@ -329,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. @@ -437,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, ) } } @@ -458,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/PathGuard.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuard.kt index bb3fec02..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 @@ -203,6 +203,30 @@ 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. + * @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 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 { + // 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 } + } + /** * 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/UpdateFileHandler.kt b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/UpdateFileHandler.kt index d93352cb..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 @@ -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 { @@ -59,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/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..f71c154d --- /dev/null +++ b/plugins/AI-Core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLines.kt @@ -0,0 +1,95 @@ +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. + * @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 + + /** Letters of any script, with their combining marks, so a localized string still matches. */ + 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 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 = 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 = 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 } + 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 } + .take(MAX_LINES) + .map { (line, _) -> line } + .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. + * @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 -> + 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; a tab is shown as \\t):\n$body" + } +} 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..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 @@ -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() } @@ -86,6 +89,19 @@ internal class AgentRunReporter( onAskedToFinish() } + override suspend fun onUnappliedWriteAnswer(turn: Int) { + 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) { + 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 d52a838c..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 @@ -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 @@ -226,6 +227,7 @@ class ChatViewModel( terminalTool = RESPOND_TOOL, unfinishedTurn = toolResultsPrompt::unfinished, requiredToolTurn = toolResultsPrompt::requiredTool, + unappliedWriteTurn = toolResultsPrompt::unappliedWrite, ) val toolExecutionTracker = ToolExecutionTracker() @@ -1151,6 +1153,19 @@ 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 -> + val handler = tools.router.getHandler(call.name) + 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 + }, canonicalName = { name -> tools.router.getHandler(name)?.toolName ?: name }, requiredTool = requiredTool, // Only a prose turn that showed an answer has one a recap could repeat. @@ -2035,6 +2050,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..1df0c9a0 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. + 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/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 e3fc09b3..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 @@ -951,8 +951,584 @@ 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(maxIterations: Int = AgentLoop.DEFAULT_MAX_ITERATIONS) = AgentLoop( + formatToolResults = ToolResultsPrompt({ shippedConfig }, "respond"), + maxIterations = maxIterations, + terminalTool = "respond", + unfinishedTurn = { UNFINISHED }, + unappliedWriteTurn = { UNAPPLIED }, + ) + + /** + * 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, 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() + val events = mutableListOf() + val history = mutableListOf(ChatMessage(Role.USER, "change the app name")) + val pathsOf = { call: ToolCall -> setOfNotNull(call.args["path"]?.toString()) } + val result = loop.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() } } + .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" } + 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") + private val madeEdit = ToolResult(false, "old_string not found", madeIfUnchanged = true) + + @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", "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(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"))), + ) + + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "not-made"), 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 givenAFailedEditFollowedByASuccessfulOneOnTheSameFile_whenTheModelAnswers_thenNothingIsOutstanding() = runTest { + val (result, _, events) = runWriting( + listOf(toolCall("edit_file", "a"), toolCall("edit_file", "a"), respond), + mapOf("edit_file" to listOf(failedEdit, appliedEdit)), + ) + + assertTrue(result.completed) + 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"))), + ) + + // Told, then asked to carry on, and still only talking: the change was not made. + assertEquals(AgentLoop.StopReason.ABANDONED, result.reason) + assertEquals(listOf("unapplied", "unfinished", "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) + } + + @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 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)), + ) + + 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, madeEdit)), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + fun givenAnEditThatApplied_whenTheModelIssuesItAgainAfterALook_thenTheRepeatFailingIsNotAChangeNotMade() = runTest { + // 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, madeEdit), "read_file" to listOf(ToolResult.success("lines"))), + ) + + assertTrue(result.completed) + assertEquals(listOf("final"), events) + } + + @Test + 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 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. + 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, 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. + val (result, _, events) = runWriting( + listOf(edit("a", "Hello"), edit("a", "subtitle"), edit("a", "Hello"), respond, respond), + mapOf("edit_file" to listOf(appliedEdit, appliedEdit, madeEdit)), + ) + + 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. + val (result, _, events) = runWriting( + 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, madeEdit)), + ) + + 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. + 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( + 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" const val REQUIRED = "call first:" } } 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..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) { @@ -495,9 +498,22 @@ 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")) } + @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/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/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/CreateFileHandlerTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/CreateFileHandlerTest.kt new file mode 100644 index 00000000..60f5c095 --- /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 a547a3e8..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 @@ -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 @@ -149,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", ) @@ -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) @@ -642,6 +673,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/PathGuardTest.kt b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/PathGuardTest.kt index 16a2044a..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 @@ -185,6 +185,72 @@ 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) + .flatMap { path -> listOf(true, false).map { PathGuard.writeKeyOf(path, correctsNames = it) } } + + assertEquals(listOf(file.canonicalPath), keys.distinct()) + } + + @Test + 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", correctsNames = true)) + } + + @Test + 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( + 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 + 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", 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 -------------------- @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 new file mode 100644 index 00000000..5f67f9fe --- /dev/null +++ b/plugins/AI-Core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/tool/handlers/edit/NearMatchLinesTest.kt @@ -0,0 +1,156 @@ +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_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, line.text.length) + assertTrue(line.cut) + 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" + + 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 + 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 \"")) + } + + @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 }) + } + + @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 }) + } + + @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 }) + } +}