fix(tools): tolerate indent drift + content alias in file_edit; concrete scope-widen remedy
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rdo9fe7SujNVeyZA8YkpkD
This commit is contained in:
+6
-2
@@ -78,8 +78,12 @@ class ManifestContainmentRule : ToolCallRule {
|
|||||||
val remedy = if (taskScope.isEmpty()) {
|
val remedy = if (taskScope.isEmpty()) {
|
||||||
" Claim a task whose affected_paths cover this path (task_update), then retry."
|
" Claim a task whose affected_paths cover this path (task_update), then retry."
|
||||||
} else {
|
} else {
|
||||||
" If this path is genuinely part of the task, widen its affected_paths via " +
|
val taskId = input.session.activeTask?.taskId
|
||||||
"task_update (note why), then retry."
|
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(
|
issues += ValidationIssue(
|
||||||
code = "PATH_OUTSIDE_MANIFEST",
|
code = "PATH_OUTSIDE_MANIFEST",
|
||||||
|
|||||||
@@ -52,11 +52,15 @@ class WriteScopeRule : ToolCallRule {
|
|||||||
)
|
)
|
||||||
|
|
||||||
if (!inScope) {
|
if (!inScope) {
|
||||||
|
val widened = (active.scope + raw).joinToString(", ") { "\"$it\"" }
|
||||||
issues += ValidationIssue(
|
issues += ValidationIssue(
|
||||||
code = RULE_CODE,
|
code = RULE_CODE,
|
||||||
message = "Tool '${input.request.toolName}' writes '$raw', outside claimed task " +
|
message = "Tool '${input.request.toolName}' writes '$raw', outside claimed task " +
|
||||||
"${active.taskId}'s affected_paths (${active.scope}). If this change is needed, " +
|
"${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,
|
severity = ValidationSeverity.ERROR,
|
||||||
)
|
)
|
||||||
disposition = maxAction(disposition, RiskAction.BLOCK)
|
disposition = maxAction(disposition, RiskAction.BLOCK)
|
||||||
|
|||||||
@@ -59,6 +59,7 @@ class WriteScopeRuleTest {
|
|||||||
assertEquals(RiskAction.BLOCK, r.disposition)
|
assertEquals(RiskAction.BLOCK, r.disposition)
|
||||||
assertEquals("WRITE_SCOPE", r.issues.single().code)
|
assertEquals("WRITE_SCOPE", r.issues.single().code)
|
||||||
assertTrue(r.issues.single().message.contains("auth-2"))
|
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=["))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+64
-17
@@ -148,11 +148,11 @@ class FileEditTool(
|
|||||||
operation == "append" && !request.parameters.containsKey("content") ->
|
operation == "append" && !request.parameters.containsKey("content") ->
|
||||||
ValidationResult.Invalid("Missing 'content' parameter for 'append' operation.")
|
ValidationResult.Invalid("Missing 'content' parameter for 'append' operation.")
|
||||||
|
|
||||||
operation == "replace" && (
|
operation == "replace" && missingReplaceParams(request) ->
|
||||||
!request.parameters.containsKey("target") ||
|
ValidationResult.Invalid(
|
||||||
!request.parameters.containsKey("replacement")
|
"Missing 'target' or 'replacement' parameter for 'replace' operation. " +
|
||||||
) ->
|
"Provide target=\"exact string to find\" and replacement=\"new string\".",
|
||||||
ValidationResult.Invalid("Missing 'target' or 'replacement' parameter for 'replace' operation.")
|
)
|
||||||
|
|
||||||
// Anchor validity is checkable now, so reject a bad target BEFORE the approval gate
|
// 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
|
// 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 =
|
private fun isPathAllowed(path: Path): Boolean =
|
||||||
PathJail.isContained(path, normalizedAllowedPaths)
|
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 {
|
private fun validateReplaceAnchor(path: Path, pathString: String, request: ToolRequest): ValidationResult {
|
||||||
val target = request.parameters["target"] as? String ?: return ValidationResult.Valid
|
val target = request.parameters["target"] as? String ?: return ValidationResult.Valid
|
||||||
val content = Files.readString(path)
|
val content = Files.readString(path)
|
||||||
return when (val occurrences = content.split(target).size - 1) {
|
return when (val occurrences = content.split(target).size - 1) {
|
||||||
1 -> ValidationResult.Valid
|
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))
|
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 =
|
private fun targetNotFoundMessage(pathString: String, content: String, target: String): String =
|
||||||
buildString {
|
buildString {
|
||||||
append("Target not found in $pathString")
|
append("Target not found in $pathString")
|
||||||
@@ -281,23 +314,29 @@ class FileEditTool(
|
|||||||
|
|
||||||
private fun replace(path: Path, pathString: String, request: ToolRequest): ToolResult {
|
private fun replace(path: Path, pathString: String, request: ToolRequest): ToolResult {
|
||||||
val target = request.parameters["target"] as String
|
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 currentContent = Files.readString(path)
|
||||||
|
|
||||||
val occurrences = currentContent.split(target).size - 1
|
val occurrences = currentContent.split(target).size - 1
|
||||||
return when (occurrences) {
|
return when (occurrences) {
|
||||||
1 -> {
|
1 -> {
|
||||||
val newContent = currentContent.replace(target, replacement)
|
val newContent = currentContent.replace(target, replacement)
|
||||||
AtomicFileWriter.write(path, newContent.toByteArray(Charsets.UTF_8))
|
writeReplaced(path, pathString, newContent, request)
|
||||||
ToolResult.Success(
|
|
||||||
invocationId = request.invocationId,
|
|
||||||
output = "Target replaced in $pathString",
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
// Don't echo the (often whole-file) target back into context — it bloats the turn and
|
// Exact miss is almost always indent drift — retry ignoring per-line whitespace, and
|
||||||
// teaches nothing. Instead point at the closest actual line, since a miss is almost
|
// rebase the replacement onto the file's real indentation so the result stays well-formed.
|
||||||
// always whitespace/content drift the model can correct from a nudge.
|
0 -> flexibleMatch(currentContent, target)?.let { range ->
|
||||||
0 -> ToolResult.Failure(
|
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,
|
invocationId = request.invocationId,
|
||||||
reason = targetNotFoundMessage(pathString, currentContent, target),
|
reason = targetNotFoundMessage(pathString, currentContent, target),
|
||||||
recoverable = true,
|
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
|
* 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
|
* non-blank line and return the file line most similar to it (levenshtein ratio), or null when
|
||||||
|
|||||||
+50
@@ -186,6 +186,56 @@ class FileEditToolTest {
|
|||||||
assertEquals("the quick red fox", Files.readString(filePath))
|
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
|
@Test
|
||||||
fun `execute replace failure zero matches`(): Unit = runBlocking {
|
fun `execute replace failure zero matches`(): Unit = runBlocking {
|
||||||
val tempDir = Files.createTempDirectory("file_edit_replace_fail")
|
val tempDir = Files.createTempDirectory("file_edit_replace_fail")
|
||||||
|
|||||||
Reference in New Issue
Block a user