From 1acb5cc8ff8edeae40081c0064eca4319942867d Mon Sep 17 00:00:00 2001 From: kami Date: Tue, 21 Jul 2026 00:25:54 +0400 Subject: [PATCH] fix(tools): tolerate indent drift + content alias in file_edit; concrete scope-widen remedy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit file_edit failed en masse for small models on two ergonomics traps: - replace() rejected calls that sent `content` (the append param) instead of `replacement` — intent unambiguous; now accepted as an alias. - exact-string match died on leading-whitespace drift (model can't reproduce indentation). Added whitespace-flexible line matching + replacement reindent, wired into both the pre-exec validation gate and replace(). Also made the WRITE_SCOPE / PATH_OUTSIDE_MANIFEST block messages emit a literal copy-pasteable task_update(id=..., affected_paths=[...]) call and warn against action=block — the exact wrong turn models kept taking (18x in one session). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Rdo9fe7SujNVeyZA8YkpkD --- .../rules/ManifestContainmentRule.kt | 8 +- .../core/toolintent/rules/WriteScopeRule.kt | 6 +- .../core/toolintent/WriteScopeRuleTest.kt | 3 +- .../tools/filesystem/FileEditTool.kt | 81 +++++++++++++++---- .../tools/filesystem/FileEditToolTest.kt | 50 ++++++++++++ 5 files changed, 127 insertions(+), 21 deletions(-) diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt index 46782667..57ef6242 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt @@ -78,8 +78,12 @@ class ManifestContainmentRule : ToolCallRule { val remedy = if (taskScope.isEmpty()) { " Claim a task whose affected_paths cover this path (task_update), then retry." } else { - " If this path is genuinely part of the task, widen its affected_paths via " + - "task_update (note why), then retry." + val taskId = input.session.activeTask?.taskId + val widened = (taskScope + raw).joinToString(", ") { "\"$it\"" } + " If this path is genuinely part of the task, widen scope with this exact call — " + + "task_update(id=\"$taskId\", affected_paths=[$widened], note=\"why $raw is in " + + "scope\") — then retry the write. affected_paths REPLACES the old list, so keep " + + "the existing paths. Do NOT use action=block/unblock; that does not change scope." } issues += ValidationIssue( code = "PATH_OUTSIDE_MANIFEST", diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt index 85dd7438..b1729845 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt @@ -52,11 +52,15 @@ class WriteScopeRule : ToolCallRule { ) if (!inScope) { + val widened = (active.scope + raw).joinToString(", ") { "\"$it\"" } issues += ValidationIssue( code = RULE_CODE, message = "Tool '${input.request.toolName}' writes '$raw', outside claimed task " + "${active.taskId}'s affected_paths (${active.scope}). If this change is needed, " + - "add its path via task_update affected_paths (note why), then retry.", + "widen the scope with this exact call — task_update(id=\"${active.taskId}\", " + + "affected_paths=[$widened], note=\"why $raw is in scope\") — then retry the write. " + + "The affected_paths list REPLACES the old one, so include the existing paths shown " + + "above. Do NOT use action=block/unblock; that does not change scope.", severity = ValidationSeverity.ERROR, ) disposition = maxAction(disposition, RiskAction.BLOCK) diff --git a/core/toolintent/src/test/kotlin/com/correx/core/toolintent/WriteScopeRuleTest.kt b/core/toolintent/src/test/kotlin/com/correx/core/toolintent/WriteScopeRuleTest.kt index 6f78db70..388247a5 100644 --- a/core/toolintent/src/test/kotlin/com/correx/core/toolintent/WriteScopeRuleTest.kt +++ b/core/toolintent/src/test/kotlin/com/correx/core/toolintent/WriteScopeRuleTest.kt @@ -59,6 +59,7 @@ class WriteScopeRuleTest { assertEquals(RiskAction.BLOCK, r.disposition) assertEquals("WRITE_SCOPE", r.issues.single().code) assertTrue(r.issues.single().message.contains("auth-2")) - assertTrue(r.issues.single().message.contains("task_update affected_paths")) + assertTrue(r.issues.single().message.contains("task_update(id=\"auth-2\"")) + assertTrue(r.issues.single().message.contains("affected_paths=[")) } } diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt index 870bfd2b..57e71423 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt @@ -148,11 +148,11 @@ class FileEditTool( operation == "append" && !request.parameters.containsKey("content") -> ValidationResult.Invalid("Missing 'content' parameter for 'append' operation.") - operation == "replace" && ( - !request.parameters.containsKey("target") || - !request.parameters.containsKey("replacement") - ) -> - ValidationResult.Invalid("Missing 'target' or 'replacement' parameter for 'replace' operation.") + operation == "replace" && missingReplaceParams(request) -> + ValidationResult.Invalid( + "Missing 'target' or 'replacement' parameter for 'replace' operation. " + + "Provide target=\"exact string to find\" and replacement=\"new string\".", + ) // Anchor validity is checkable now, so reject a bad target BEFORE the approval gate // fires — mirroring read/write's file-exists / read-before-write pre-checks. Without @@ -180,20 +180,53 @@ class FileEditTool( } } + /** A replace needs a target and a replacement; 'content' is accepted as a replacement alias. */ + private fun missingReplaceParams(request: ToolRequest): Boolean = + !request.parameters.containsKey("target") || + (!request.parameters.containsKey("replacement") && !request.parameters.containsKey("content")) + private fun isPathAllowed(path: Path): Boolean = PathJail.isContained(path, normalizedAllowedPaths) - /** Valid iff the replace target occurs exactly once. Same 0/>1 messaging as [replace] at execute. */ + /** Valid iff the replace target occurs exactly once (exact, else whitespace-flexible). */ private fun validateReplaceAnchor(path: Path, pathString: String, request: ToolRequest): ValidationResult { val target = request.parameters["target"] as? String ?: return ValidationResult.Valid val content = Files.readString(path) return when (val occurrences = content.split(target).size - 1) { 1 -> ValidationResult.Valid - 0 -> ValidationResult.Invalid(targetNotFoundMessage(pathString, content, target)) + 0 -> if (flexibleMatch(content, target) != null) ValidationResult.Valid + else ValidationResult.Invalid(targetNotFoundMessage(pathString, content, target)) else -> ValidationResult.Invalid(targetAmbiguousMessage(pathString, occurrences)) } } + /** + * Locate [target] in [content] ignoring each line's leading/trailing whitespace — small models + * routinely drop or misjudge indentation, so an exact-string miss is almost always an indent + * mismatch, not a wrong edit. Returns the matched file-line range iff exactly one block matches. + * ponytail: line-trim match only; mixed tab/space or a target spanning blank-line drift may miss. + */ + private fun flexibleMatch(content: String, target: String): IntRange? { + val fileLines = content.split("\n") + val targetLines = target.split("\n").dropLastWhile { it.isBlank() } + if (targetLines.isEmpty() || targetLines.size > fileLines.size) return null + val normTarget = targetLines.map { it.trim() } + val starts = (0..fileLines.size - targetLines.size).filter { start -> + normTarget.indices.all { fileLines[start + it].trim() == normTarget[it] } + } + return if (starts.size == 1) starts[0] until (starts[0] + targetLines.size) else null + } + + /** Rebase [replacement]'s indentation onto [baseIndent], preserving its own relative structure. */ + private fun reindent(replacement: String, baseIndent: String): String { + val lines = replacement.split("\n") + val replBase = lines.firstOrNull { it.isNotBlank() }?.takeWhile { it == ' ' || it == '\t' }.orEmpty() + return lines.joinToString("\n") { line -> + if (line.isBlank()) line + else baseIndent + line.removePrefix(replBase).let { if (it == line) line.trimStart() else it } + } + } + private fun targetNotFoundMessage(pathString: String, content: String, target: String): String = buildString { append("Target not found in $pathString") @@ -281,23 +314,29 @@ class FileEditTool( private fun replace(path: Path, pathString: String, request: ToolRequest): ToolResult { val target = request.parameters["target"] as String - val replacement = request.parameters["replacement"] as String + // Small models routinely send 'content' (the append param) on a replace; the intent is + // unambiguous, so accept it as replacement rather than rejecting a clearly-correct edit. + val replacement = (request.parameters["replacement"] ?: request.parameters["content"]) as String val currentContent = Files.readString(path) val occurrences = currentContent.split(target).size - 1 return when (occurrences) { 1 -> { val newContent = currentContent.replace(target, replacement) - AtomicFileWriter.write(path, newContent.toByteArray(Charsets.UTF_8)) - ToolResult.Success( - invocationId = request.invocationId, - output = "Target replaced in $pathString", - ) + writeReplaced(path, pathString, newContent, request) } - // Don't echo the (often whole-file) target back into context — it bloats the turn and - // teaches nothing. Instead point at the closest actual line, since a miss is almost - // always whitespace/content drift the model can correct from a nudge. - 0 -> ToolResult.Failure( + // Exact miss is almost always indent drift — retry ignoring per-line whitespace, and + // rebase the replacement onto the file's real indentation so the result stays well-formed. + 0 -> flexibleMatch(currentContent, target)?.let { range -> + val fileLines = currentContent.split("\n") + val baseIndent = fileLines[range.first].takeWhile { it == ' ' || it == '\t' } + val newLines = fileLines.toMutableList() + repeat(range.count()) { newLines.removeAt(range.first) } + newLines.addAll(range.first, reindent(replacement, baseIndent).split("\n")) + writeReplaced(path, pathString, newLines.joinToString("\n"), request) + } ?: ToolResult.Failure( + // Don't echo the (often whole-file) target back into context — it bloats the turn. + // Point at the closest actual line instead. invocationId = request.invocationId, reason = targetNotFoundMessage(pathString, currentContent, target), recoverable = true, @@ -310,6 +349,14 @@ class FileEditTool( } } + private fun writeReplaced(path: Path, pathString: String, newContent: String, request: ToolRequest): ToolResult { + AtomicFileWriter.write(path, newContent.toByteArray(Charsets.UTF_8)) + return ToolResult.Success( + invocationId = request.invocationId, + output = "Target replaced in $pathString", + ) + } + /** * Best-effort "did you mean" for a not-found replace target: anchor on the target's first * non-blank line and return the file line most similar to it (levenshtein ratio), or null when diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileEditToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileEditToolTest.kt index be0d0d06..6e5d9fd0 100644 --- a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileEditToolTest.kt +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileEditToolTest.kt @@ -186,6 +186,56 @@ class FileEditToolTest { assertEquals("the quick red fox", Files.readString(filePath)) } + @Test + fun `replace tolerates indentation drift and rebases the replacement`(): Unit = runBlocking { + // The model dropped the file's leading indentation in its target (the dominant Mode-2 + // failure). Flexible match must locate the block and re-indent the replacement to match. + val tempDir = Files.createTempDirectory("file_edit_indent") + val filePath = tempDir.resolve("Card.tsx") + Files.writeString( + filePath, + "interface CardProps {\n children: React.ReactNode;\n className?: string;\n}\n", + ) + val tool = FileEditTool(allowedPaths = setOf(tempDir)) + val request = createRequest( + mapOf( + "operation" to "replace", + "path" to filePath.toString(), + // No leading indentation, exactly as the model emitted it. + "target" to "children: React.ReactNode;\nclassName?: string;", + "replacement" to "children: React.ReactNode;\nclassName?: string;\ntitle?: string;", + ), + ) + + val result = tool.execute(request) + assertTrue(result is ToolResult.Success, "indent-drift replace should succeed") + // Every replaced line rebased to the file's real 2-space indent, including the new one. + assertEquals( + "interface CardProps {\n children: React.ReactNode;\n className?: string;\n title?: string;\n}\n", + Files.readString(filePath), + ) + } + + @Test + fun `replace accepts content as an alias for replacement`(): Unit = runBlocking { + val tempDir = Files.createTempDirectory("file_edit_alias") + val filePath = tempDir.resolve("test.txt") + Files.writeString(filePath, "the quick brown fox") + val tool = FileEditTool(allowedPaths = setOf(tempDir)) + val request = createRequest( + mapOf( + "operation" to "replace", + "path" to filePath.toString(), + "target" to "brown", + "content" to "red", // model sent the append-style key on a replace + ), + ) + + val result = tool.execute(request) + assertTrue(result is ToolResult.Success) + assertEquals("the quick red fox", Files.readString(filePath)) + } + @Test fun `execute replace failure zero matches`(): Unit = runBlocking { val tempDir = Files.createTempDirectory("file_edit_replace_fail")