[davidschachterADFA]

Summary

Decision (2026-10-29): for the 1.5B on-device model, an honest "the change was not made" is the accepted outcome; ADFA-6835 closes when this merges. Whether a larger model (3B) does the edit is measured separately in ADFA-6839.

ADFA-6835. This does not make the on-device model complete the edit (see "Measured"); it makes the failure show the model what to copy, and stops the run reporting a change that never happened. Eight commits, one per behaviour, the last five from review:

• Closest lines in the rejection (4ababd1). When edit_file rejects an edit as "old_string not found" or "identical to new_string", the message now lists up to five lines of the file that share the most words with the guess: line number and exact text between quotes, trailing spaces included. The identical-strings check moved to after the file is read so it can carry the lines too (a missing or protected path is now reported before it). New NearMatchLines; it works by shared words, never regex. Model-independent, so every backend gets it.
• Tell the run its change failed (0ea705d). If a run answers while a file change it attempted failed and has not been redone, it is told once, in agent_loop.yml, that the change was not made, to read and retry or say so plainly.
• Never end it as done (f4e3196). A run that still ends with a failed change outstanding, by answering again or by the repeat-after-success rule, is now ABANDONED instead of COMPLETED, and a notice says the change was not made.
• Track each failed change by path (aaecc13, from Daniel's review). The run-wide flag became a set of paths. Only edit_file, create_file and update_file add to it (new ToolHandler.writesFiles), so a shell command or add_dependency neither sets nor clears it. A success or a user denial clears its own path. The warning re-arms once nothing is outstanding. A prose answer gets the same check as respond. The notice now starts "A change was not made: a file edit failed and …".
• Key a failed change by its file; tell the user however the run ends (6a93052, from an independent review of aaecc13). A failure is keyed by the file, not the path as typed (PathGuard.writeKeyOf), so a retry under the path edit_file corrected a bare name to now counts as the redo. A correction or an approval timeout returns the new ToolResult.notApproved and is settled like a denial. A success settles only failures from earlier batches, so a failed edit beside a successful one on the same file stays outstanding. After the warning, prose gets the unfinished-turn nudge before the run is ended. A repeated look warns first instead of ending at once, and does nothing when the check is off. Runs that stop as UNPARSABLE, CYCLING, REPEATED or MAX_ITERATIONS now show the notice too. The closest-lines hint scores whole lines and marks a line cut at 200 characters, shows a tab as \t, matches words in any script, and scores at most 32 words.
• Key a failed write as its own tool resolves the path (b869b92, from a second independent review, of 6a93052). Only edit_file corrects a missing path to the one project file of that name (new ToolHandler.correctsPaths), so a failed create_file or update_file is no longer keyed to another file of the same name; it falls back to the bare name, and skips that project walk. A path with no file name (app/) keeps its own key instead of the empty one that any later write settles. A line cut at 200 characters no longer splits a surrogate pair.
• Let a warned run redo its change (b0a09c4, from a third independent review, of b869b92). After the warning, the run may repeat its last batch once, so a model that obeys by reading again the file it just read gets a turn to make the edit; a second repeat still ends the run with the notice. An approval timeout is a plain failure again: nobody decided against the call, so a failed change it was redoing stays outstanding (a correction is still settled like a denial). A write that already worked, issued again in a later batch, fails because it made the change and is no longer counted as not made. create_file and update_file key a failure by the path they were given even when the file does not exist yet, so a write to another strings.xml no longer settles a failed create of values-es/strings.xml; only a path outside the project falls back to the bare name. The empty bare name of app/ no longer settles the failure of a call that named no file. Keys are looked up only while a failure is outstanding. The warning now says only the failed change was not made, says not to redo other changes to that file, and gives each write tool its recovery; the notice says "a file change failed". The README lists the unapplied_write turn.
• Count a repeat as failed once another write changed its file; match whole words (c1a9810, from a fourth independent review, of b0a09c4). A write that already worked and fails again is no longer taken for the change already made when a different write to the same file worked in between, which may have undone it; the file lookup runs only for such a call. Recording writes does nothing when the check is off. The closest-lines hint matches whole words, so val no longer scores a line holding evaluate.
• Count a repeated write that already worked as made; match parts of identifiers and unspaced scripts (82ed8fa, from a fifth independent review, of c1a9810). The repeat rule from c1a9810 is removed: it compared bare file names, so values/strings.xml and values-es/strings.xml counted as one file. (Its stated reason, that a write undoing a change would let the repeat succeed, was wrong: ddbd07e below.) The closest-lines hint also matches a part of an identifier (name finds app_name and appName) and, in a script written without spaces, a part of a phrase. val still misses evaluate.
• Excuse a failed repeat only when nothing has written its file since it worked (ddbd07e, from a sixth independent review, of 82ed8fa, and three more of the fixes in between, squashed). 82ed8fa took any failed repeat of a write that had worked as made. A later write can undo the change in a form the repeat cannot find: a whole-file update_file from a stale read re-indents the old text back, and the run ended COMPLETED with "Done" over a title that still said Hello. Three ways of reading the answer off the file's text were each fooled by some edit, so nothing is read off it now.
  • A failed repeat is excused only for the same change (ToolRouter.changeOf: the tool and its declared changeArgs, read through its aliases) to the same file (ToolResult.file, the canonical path the tool resolved).
  • edit_file "old_string not found" (madeIfUnchanged) is excused only when no successful write to that file, and no project change naming no file, came since.
  • create_file on an existing file (alreadyMade) is excused whatever came since.
  • Accepted costs, both toward telling the user to check, never toward a false "done": a repeat after the model changed the same file again, or after any shell command or MCP tool (even one that only reads), is reported not made; a repeated create is taken for made even if the file was rewritten since.
• Test the repeat check with the real file tools (88230c5, from a seventh independent review, of ddbd07e). AgentLoopFileToolsTest drives the loop with the real edit, update and create tools through a ToolRouter, as ChatViewModel wires it.

Measured on a Pixel 6 Pro (Android 17)

qwen2.5-coder-1.5b-instruct-q4_k_m, "Build Apps Anywhere" test project, a fresh chat, "Change the app name in strings.xml to Block Stacker", every write declined, three runs per build. Builds: main plus this branch; AI-Agent-Local from #155's branch (596cb7c for the b0a09c4 runs).

Build	Runs
Before (on #155's build)	3 of 3: edit_file ×3 with the same identical guess, rejected each time; REPEATED; read_file never called
Closest lines only	3 of 3: 2nd call copies part of the real line but not exactly (old_string rejected again); then read_file; then respond "has been changed" (false)
+ tell the run	3 of 3: guard fires; the model re-reads the file; the repeat-after-success rule ended it COMPLETED with the false answer standing
+ never end it as done (f4e3196)	3 of 3: same calls; ABANDONED after 5 turns, and the chat shows "The change was not made: the file edit failed and …"
+ review fixes (b0a09c4)	2 of 3: the same four calls, then after the warning the model answers "The change to strings.xml was not made." instead of re-reading; ABANDONED after 5 turns. 1 of 3: one rejected edit, then "already set to 'Block Stacker'" (false), then after the warning "was not made. Please try again."; ABANDONED after 3 turns. All 3 show the notice

• No run reached the approval dialog, on f4e3196 or b0a09c4. The model answers instead of calling edit_file again, so this PR does not meet ADFA-6835's "done when". The remaining gap is the model, not the handler.
• On b0a09c4 the model says itself that the change was not made (3 of 3), where on f4e3196 it re-read the file. So the one repeat allowed after the warning was never used on the phone; it is unit-tested only.
• In the 3-turn run the chat shows both answers as "I couldn't complete that, the last action failed. See the details above." That is main's rule (AgentReplyRenderer, from ADFA-6339 | Let the agent run, read and stop shell commands in the Terminal #125) for an answer that follows a failed call; it hides the model's own "was not made" too.
• Not covered: a model that goes straight to "changed" with no edit call at all (one run did this earlier, before the guard); nothing in the loop sees that.
• On the phone the notice appears in the chat's Log strip, truncated ("A change was not made: a file change failed and th…"); I did not check the expanded text.

88230c5 (2026-10-31): qwen2.5-0.5b-instruct-q4_k_m, because the 1.5B file was no longer on the phone. JitpackDemoTest project, a fresh chat per run, writes approved.

• Two runs of "Change the app name in strings.xml to Block Stacker": 2 and 3 guesses, each rejected before the approval dialog, then REPEATED, with both notices ("kept requesting the same action", "A change was not made"). The file was unchanged.
• One run of "In strings.xml replace JitpackDemoTest with Block Stacker": the first edit_file named a wrong path (app/src/main/strings.xml). The tool corrected it, and the edit applied once approved. The model then sent it twice more with the corrected full path. Both failed because the change was already there. The run stopped REPEATED with no "change not made" notice, and strings.xml held the change. This is a repeat under another spelling of the path, which 82ed8fa would have reported as not made.

Review

Daniel reviewed f4e3196 and found six IMPORTANT defects in the flag. aaecc13 fixes all six, each with a test that fails on f4e3196. The loop change is still the part to read: AgentLoop.kt (UnappliedWrites, the prose and terminal-tool branches, and the ASSUME_COMPLETE verdict). One notable choice: the terminal-tool block is no longer inside terminalTool?.let, with the same behaviour when no terminal tool is set.

An independent review of aaecc13 (the code-review skill, given only the PR) raised 15 findings. 6a93052 fixes 12, see above; 17 of its 18 new tests fail with the old behaviour put back, and the 18th guards against matching two files that share a name. A second independent review, of 6a93052 (a Sonnet 5.5 agent given the code and the earlier findings), raised four real low-severity findings, fixed in b869b92; each of its four new tests fails with the old behaviour put back.

A third independent review, of b869b92 (the code-review skill, given only the PR), raised 15 findings; I checked each against the code (13 confirmed, 2 plausible). b0a09c4 fixes seven of them, plus the warning wording, which David approved; each of its eight new tests fails with the old behaviour put back. Two of its findings go to ADFA-6847: keying a failed write from the file the handler resolved instead of re-deriving it (correctsPaths, writeKeyOf), and one approval-outcome value instead of the denied/notApproved flags.

Not changed:

• An edit whose old and new text are the same and already in the file is still rejected, though the first reviewer suggested treating it as done. Asked to change a value, the 1.5B model sends the old line as both, and success there would bring back the false "done". The identical-strings check stays after the path check (third review), so the rejection can carry the closest lines.
• The warned answer is already on screen before the loop decides to warn, so the user can see the model's "done" (or "couldn't complete") and later the notice. A UI choice for a reviewer.
• The closest-lines hint is also shown to the user, in the chat's error message for the failed call, with up to five lines of their file. Kept on purpose: David chose to show it.
• The hint lists single lines by score, so for a multi-line old_string a model could copy one line and leave the rest behind. Not seen; to watch for on the phone.
• A tab is shown as \t, which looks the same as a literal backslash-t in source.

Accepted limitations: an edit redone through the shell (sed -i) does not count as redone, so that run gets one warning it does not need. A success on a file in a later batch is taken for the redo of any earlier failure on that file, even if it was a different change: which change a retry redoes cannot be read off its text, and guessing wrong the other way would tell the user a change that was made was not.

Not verified

• One phone, one model file, three runs of one prompt per build.
• Bigger models and the cloud backends: unit tests only. Their behaviour after the warning is unmeasured.
• The warning's wording is untuned on a device. A model might follow it better with different text.
• The user-denied and timed-out cases are covered by unit tests, not on a device.
• The phone results are from f4e3196 and b0a09c4; c1a9810, 82ed8fa and ddbd07e are unit-tested only. 88230c5 was run on the phone (above) with the 0.5B model, so its runs don't compare directly with the 1.5B ones. aaecc13, 6a93052 and b869b92 were not run on the phone by themselves; b0a09c4 includes them.
• The repeat allowed after the warning, and a redo after it, have not happened on a device.

Test plan

• AI-Core testDebugUnitTest on 88230c5 (1,339 tests, 0 failures); every mutant of the repeat rule, its flags and its key, from the reviews and my own, fails a test
• Review probes driving the loop with the real handlers: all pass but three, each tracked or accepted (a repeated insert applying twice, ADFA-6879; a repeat after the model's own later edit; a repeat retyped with other indentation)
• assemblePlugin on 88230c5
• addons check exits 0
• Same-phone check above
• Independent review (Daniel, f4e3196); fixes in aaecc13 await his re-review
• Independent review (code-review skill, aaecc13); fixes in 6a93052
• Independent review (Sonnet 5.5 agent, 6a93052); fixes in b869b92
• Independent review (code-review skill, b869b92); fixes in b0a09c4, follow-up ADFA-6847
• Independent review (code-review skill, b0a09c4); fixes in c1a9810, the rest to ADFA-6847
• Independent review (code-review skill, c1a9810); fixes in 82ed8fa
• Independent reviews (agents) of 82ed8fa and of three fixes after it; the fixes are squashed into ddbd07e; closest-lines hint gaps to ADFA-6878, older repeat defects to ADFA-6879
• Independent review (agent, ddbd07e); tests in 88230c5, MCP read-only tools to ADFA-6880
• Phone check of 88230c5 (3 runs, above)
• Phone re-run on b0a09c4 (3 runs, all ABANDONED with the notice; none reached the approval dialog)

🤖 Generated with Claude Code

https://claude.ai/code/session_01Vh2DAaJxvRXKRYkgiDqwGQ

https://claude.ai/code/session_01VKUthHVokuWKgP3ACLBii7

[Daniel-ADFA]

IMPORTANT

• AgentLoop.kt:251 - one run-wide flag; the last result in a batch wins
• AgentLoop.kt:252 - the unapplied-write warning is never re-armed
• AgentLoop.kt:311 - prose answers skip the unapplied-write guard
• AgentLoop.kt:352 - any mutating tool's failure is reported as a failed file edit
• AgentLoop.kt:353 - a denied retry leaves the flag set

Checked by reading the code at f4e3196; not compiled or run. This repo has no written approve/request-changes rule, so the default applied.

[davidschachterADFA]

Fourth independent review (code-review skill, high), of b0a09c4

An independent reviewer returned 10 findings. I checked each against the code. Three are fixed in c1a9810, three go to ADFA-6847, and four need no change.

ID	Finding	Conclusion
F01	A later success on a file settles an earlier failed edit on it, even a different change	True and already an accepted limitation. Moved to ADFA-6847
F02	A repeated call that already worked is never counted as failed, even after another write to its file	True as stated, but the fix misfired and is reverted in 82ed8fa: a write that undid the change would let the repeat succeed, so the rule caught only unrelated edits (fifth review)
F03	The second terminal-tool answer ends the run without calling onFinalAnswer	True, but it only drops one ANSWER trace line. No change
F04	Recording writes keys files even when the check is off	True, but production always sets the check. Fixed in c1a9810: record returns early
F05	writeKeyOf can walk the project up to twice per failed edit	True and low. Not measured. No change
F06	Bare-name keys settle across files	True. Moved to ADFA-6847
F07	The closest-lines hint matched substrings (val scored evaluate)	True. Fixed in c1a9810: whole words only
F08	The identical-strings check now runs after the read	Deliberate, and the closest lines help exactly there. No change
F09	notApproved duplicates denied	True and latent: nothing calls copy(denied = ...). Moved to ADFA-6847
F10	generate_from_template and contributed tools are not tracked	A scope choice: only edit_file, create_file and update_file are tracked. No change

c1a9810 adds 4 tests; 3 fail on the old code. Unit-tested only, not run on a device.

Posted by Claude Code for David Schachter.

[davidschachterADFA]

Plugin review of AI-Core (/plugin-review) at c1a98107

Build, security audit and rubric score. Verdict: conditional, no hard blocker. No clause fails; 6.5 is Partial. Source was not changed. Not run on a device for this review.

Build: passes. assemblePlugin made ai-core.cgp (11.6 MB). The provenance record is clean (revision=c1a981077ff2, revision_source=git, no +dirty) and the manifest uses ${pluginVcsRevision} and ${pluginBuildTimestamp}. Compiler warnings were not kept, so none are listed. checkDebugAarMetadata and checkReleaseAarMetadata are turned off (build.gradle.kts:99-102) with a comment calling that the convention for these plugins.

Security: no committed signing material, no plaintext credentials, no XML parsers, no zip handling, no http:// URLs, no mavenLocal(). The only network use is fetch_url, with its stream closed (FetchUrlHandler.kt:115).

ID	Where	Finding	Conclusion
F17	settings.gradle.kts:31	jitpack.io is a dependency repository, but no AI-Core dependency comes from it. All eight are androidx, Material, Kotlin, coroutines, Markwon and Gson, from Google or Maven Central	Optional one-line removal. It adds a repository to the supply chain for nothing
F18	build.gradle.kts:32	The release build is signed with the debug signing config	Not a leaked key. Fine if that is the repo's norm
F19	BuiltInToolHandlers.kt:63-80	Class.forName probes of public plugin-API service classes, as a guard for an older host	Not reflection into IDE internals. A strict reading of 6.5 could still call it Partial
F20	AiCorePlugin.kt:832,853,859	Three tooltip buttons link to i/ai-top.html, i/ai-local.html and i/ai-online.html, which are not in this plugin's assets/docs	Tracked in ADFA-6863. On a phone the buttons could not be reached, and the documentation database shipped with that build lacks all three pages

Rubric: 6.1 Compatibility, 6.2 Resource discipline, 6.3 Build reproducibility, 6.4 Native binaries, 6.6 HTML documentation, 6.7 Tooltips and in-app help, 6.8 Icons and imagery, and Manifest declarations all Pass. 6.7 has one open item, F20. 6.5 No reflection is Partial (F19). Template thumbnails are N/A.

Not verified: the i/ai-*.html pages (F20), whether getSettingsEntries() needs a manifest key, tooltip wiring on dialogs and fragments, and behaviour on a device.

Posted by Claude Code for David Schachter.

[davidschachterADFA]

Fifth independent review (code-review), of c1a9810

An independent reviewer found three defects in c1a9810. Each was shown by a probe test run on both c1a9810 and b0a09c4. All three are fixed in 82ed8fa.

ID	Finding	Change in 82ed8fa
F01	The new "another write to its file came between" check compared bare file names. So values/strings.xml and values-es/strings.xml counted as one file, and repeating an edit to the first after editing the second ended the run ABANDONED, though both changes were made. This brought back the collision b0a09c4 fixed.	Gone with F02's rule. A new test covers two strings.xml files in different directories.
F02	The rule itself fired in the wrong cases. No write tool deletes a file, so a write that undid an edit would let the repeat succeed, and the repeat would never reach the rule. It fired only after unrelated edits to the same file, and reported a change that was made as not made. After a repeated create_file, its warning pointed the model at update_file, which would overwrite the model's own later edit.	The rule is removed. A repeated write that already worked counts as made, as in b0a09c4. The test that asserted the old rule now asserts this.
F03	With whole-word matching, the closest-lines hint missed app_name for a guess of name, and part of a Chinese phrase (我的应用 in 我的应用程序).	A word now also matches a part of an identifier, split at _ and camelCase. In a script written without spaces, it matches a part of a phrase. val still misses evaluate. There are two new tests.

The other part of c1a9810 stays: recording writes does nothing when the check is off.

Verified:

• AI-Core testDebugUnitTest: 1,313 tests, 0 failures.
• With c1a9810's code put back, all 4 new tests fail.
• addons check exits 0, and assemblePlugin passes.
• Not run on a device: the phone was in use.

@hal-eisen-adfa, you approved before these changes. Could you take another look at 82ed8fa?

Posted by Claude Code for David Schachter.

[davidschachterADFA]

What changed since 82ed8fa

@hal-eisen-adfa, you approved 82ed8fa. Since then, independent reviews found that it could end a run COMPLETED over a change that is not in the file, so there are two new commits. Could you take another look?

The defect. 82ed8fa took every failed repeat of a write that had worked as made. A later write to the same file can undo the change in a form the repeat can't find. A whole-file update_file from a stale read re-indents Hello back, the repeated edit fails "old_string not found", and the run ended COMPLETED with the model saying "Done" over a title that still said Hello.

ddbd07e: the rule. I tried three ways of reading from the file's text whether the change was still there. A review found an edit that fooled each one, so nothing is read from the text now:

Case	Taken for made when
edit_file "old_string not found" (madeIfUnchanged)	The same change to the same file worked earlier this run, and nothing has written that file since. A shell command, an MCP tool, or a write naming no file counts as a write to every file.
create_file on an existing file (alreadyMade)	The same create worked earlier this run, whatever came since. No write tool deletes a file.
Anything else	Never. It counts as a change not made, and the run is told to look.

"Same change" is ToolRouter.changeOf: the tool and its declared changeArgs, read through its aliases, so replace_all or an extra key doesn't matter. "Same file" is the canonical path the tool reports in the new ToolResult.file.

Accepted costs. Both errors point toward telling the user to check, never toward a false "done":

• A repeat after the model changed the same file again, or after any shell command or MCP tool, is reported as not made. That includes a command that only reads, like cat.
• A repeated create is taken for made even if the file was rewritten since.

88230c5: tests. AgentLoopFileToolsTest drives the loop with the real edit, update and create tools through a ToolRouter, as ChatViewModel wires it. It covers a stale rewrite through another spelling of the path, in the same batch, and to a file open in the editor; a repeat in the same batch; an update to another file; and the same create to another existing file.

Verified: 1,339 AI-Core tests pass. Every mutant of the rule, its flags and its key fails a test. assemblePlugin and addons check pass. Probes from the reviews, which drive the loop with the real tools, all pass except the three tracked or accepted cases below. Not run on a device yet. The phone has been in use.

Tracked separately:

• ADFA-6878: gaps in the closest-lines hint for CJK text and digit boundaries.
• ADFA-6879: a repeated insert edit writes its line twice. This predates 82ed8fa.
• ADFA-6880: MCP tools that declare readOnlyHint shouldn't count as writes.

Posted by Claude Code for David Schachter.

[hal-eisen-adfa]

Review of ddbd07e6 and 88230c5f. The rule is correct for every write the loop sees. These cases are writes it doesn't see, so a failed repeat of an edit can still be excused as "made":

ID	Sev	Finding	Evidence
F01	Med	The rule notices a write only when the tool declares mutatesProject. run_gradle_task (e.g. spotlessApply, a format task), quick_build and gradle_sync don't. Edit, then a formatting task, then a failed repeat: the repeat is excused, but the formatter may have changed the text. The flags predate this PR, but the rule now depends on them being complete.	RunGradleTaskHandler.kt (no mutatesProject); ChatViewModel.kt:1154
F02	Low	A shell command still running is counted as a write when it starts. If it started before the edit and rewrites the file afterwards, the loop never sees that write, and a failed repeat is excused.	RunShellCommandHandler.kt:121; AgentLoop.kt:449-452
F03	Low	Only successful calls count as writes. An add_dependency or contributed tool that fails after already changing files is never counted.	AgentLoop.kt:446-455
F04	Low	Changes the user makes outside tool calls (typing in the editor, an IDE save, outside git) are never seen, and nothing stops the user editing mid-run. If they do, an excused repeat can be wrong.	AgentLoop.kt UnappliedWrites; no editor lock in AI-Core
F05	Low	A contributed tool that declares itself readOnly but writes files is not counted. This relies on the plugin declaring itself honestly.	ContributedToolHandler.kt:80

[davidschachterADFA]

Thanks, @hal-eisen-adfa. I checked each finding against the head (7b8e04b; only a merge of main since 88230c5). All five hold. None is a regression from this PR; they are limits of what the loop can see. Proposed handling:

F01 (Med), fix in this PR. run_gradle_task, quick_build and gradle_sync get their own changesFiles flag that the repeat rule reads. I am not reusing mutatesProject, because it also sets runChangedProject (ChatViewModel.kt:840). A repeat after a build then counts, so the run is sent to look instead of being excused. Cost: a made edit that the model repeats after a build now gets one warning instead of a silent pass.

F03 (Low), fix in this PR. A failed call from a tool that changes the project counts as a possible write. This errs toward telling the run, the safe direction.

F02, F04, F05 (Low), documented, not fixed. A still-running shell command, edits the user makes outside tool calls, and a contributed tool that mis-declares itself readOnly are outside what the loop can observe. I will add them to the rule's KDoc as stated limits. Closing them needs an editor lock or a write watch on the project, which is a separate change.

Tests will pin F01 and F03.