fix(gates,tools): stop pinning deleted files; match edit targets across blank lines (#419, #417)

Both defects come from live session fced377e, where scaffold_frontend burned 89 turns without converging. #419 — the stage output manifest never tracked deletions. A deletion is a FileWrittenEvent with a null postImageHash, and stageWrittenPaths filtered that per-event instead of per-path, so a written-then-deleted file stayed in the manifest forever. The contract gate stamps file_exists on every entry, which made deleting — and therefore renaming, which is delete plus write — a permanent contract violation. That deadlocked against the build gate: it ordered "rename it to use the '.cjs' file extension", the agent complied, and the contract gate then failed file_exists on the .js it had just been told to remove. Keep the last mutation per path instead. Fixed in the shared function, so the build-gate toolchain probe, the review gate, and verification all stop seeing deleted files too. Moved to its own file to keep the gates file under the detekt function cap. #417 — file_edit failed 4 of 6 live calls, all "Target not found" with a correct "did you mean" suggestion attached. flexibleMatch already tolerated indentation drift but walked target lines against file lines positionally, so one blank line the model dropped shifted every later index and missed the whole block. Compare only non-blank lines and map the hit back to real file lines. Ambiguity still fails rather than picking a match, and reindent keeps handling the writeback. Guard tests both ways, each mutation-verified against the old logic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-01 00:23:51 +04:00
parent 5ed8ebd0c5
commit d1b84a1a9f
5 changed files with 277 additions and 22 deletions
@@ -364,18 +364,6 @@ internal fun SessionOrchestrator.sessionProducedBuildTarget(sessionId: SessionId
                KindContractTable.assertionsFor(kind, path).any { it.id == "imports_resolve" }
        }

-internal fun SessionOrchestrator.stageWrittenPaths(sessionId: SessionId, stageId: StageId): List<String> {
-    val events = eventStore.read(sessionId)
-    val invocationIds = events.mapNotNull { it.payload as? ToolInvocationRequestedEvent }
-        .filter { it.stageId == stageId }
-        .map { it.invocationId }
-        .toSet()
-    return events.mapNotNull { it.payload as? FileWrittenEvent }
-        .filter { it.invocationId in invocationIds && it.postImageHash != null }
-        .map { it.path }
-        .distinct()
-}
-
 /**
 * Static-first reviewer gate (role-reliability §5): for a stage that declares `static_analysis`
 * commands, run them (compiler / detekt / formatters) against its just-produced output in the
@@ -0,0 +1,33 @@
+package com.correx.core.kernel.orchestration
+
+import com.correx.core.events.events.FileWrittenEvent
+import com.correx.core.events.events.StoredEvent
+import com.correx.core.events.events.ToolInvocationRequestedEvent
+import com.correx.core.events.types.SessionId
+import com.correx.core.events.types.StageId
+
+internal fun SessionOrchestrator.stageWrittenPaths(sessionId: SessionId, stageId: StageId): List<String> =
+    stageWrittenPathsFrom(eventStore.read(sessionId), stageId)
+
+/**
+ * The files [stageId] wrote that still exist — the stage's live output manifest.
+ *
+ * Keeps only paths whose LAST mutation still has content. A deletion is a [FileWrittenEvent] with a
+ * null `postImageHash`, so filtering per-event rather than per-path leaves a written-then-deleted file
+ * in the manifest forever. Callers treat the manifest as ground truth: the contract gate stamps
+ * `file_exists` on every entry, which makes deleting — or renaming, which is delete plus write — a
+ * permanent contract violation, deadlocking against a build gate that demands one (observed live:
+ * "rename it to use the '.cjs' file extension" against `file_exists` on the `.js`).
+ */
+internal fun stageWrittenPathsFrom(events: List<StoredEvent>, stageId: StageId): List<String> {
+    val invocationIds = events.mapNotNull { it.payload as? ToolInvocationRequestedEvent }
+        .filter { it.stageId == stageId }
+        .map { it.invocationId }
+        .toSet()
+    return events.mapNotNull { it.payload as? FileWrittenEvent }
+        .filter { it.invocationId in invocationIds }
+        .associateBy { it.path } // last write per path wins
+        .filterValues { it.postImageHash != null }
+        .keys
+        .toList()
+}
@@ -0,0 +1,155 @@
+package com.correx.core.kernel.orchestration
+
+import com.correx.core.approvals.Tier
+import com.correx.core.events.events.EventMetadata
+import com.correx.core.events.events.EventPayload
+import com.correx.core.events.events.FileWrittenEvent
+import com.correx.core.events.events.StoredEvent
+import com.correx.core.events.events.ToolInvocationRequestedEvent
+import com.correx.core.events.events.ToolRequest
+import com.correx.core.events.types.EventId
+import com.correx.core.events.types.SessionId
+import com.correx.core.events.types.StageId
+import com.correx.core.events.types.ToolInvocationId
+import kotlinx.datetime.Instant
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.assertTrue
+import org.junit.jupiter.api.Test
+
+/**
+ * The stage output manifest must track deletions. Callers treat it as ground truth — the contract gate
+ * stamps `file_exists` on every entry — so a written-then-deleted path that survives here makes
+ * deleting, and therefore renaming, a permanent contract violation. Live session fced377e deadlocked
+ * exactly there: the build gate ordered "rename it to use the '.cjs' file extension", the agent
+ * complied, and the contract gate then failed `file_exists` on the `.js` it had just been told to
+ * remove — 89 turns without converging.
+ */
+class StageWrittenPathsTest {
+
+    private val stage = StageId("scaffold_frontend")
+    private val other = StageId("review_ui")
+
+    @Test
+    fun `a written-then-deleted path drops out of the manifest`() {
+        val events = listOf(
+            invoked("inv1", stage),
+            wrote("inv1", "frontend/postcss.config.js", "h1"),
+            invoked("inv2", stage),
+            deleted("inv2", "frontend/postcss.config.js"),
+        )
+        assertEquals(emptyList<String>(), stageWrittenPathsFrom(events, stage))
+    }
+
+    @Test
+    fun `a rename leaves only the new path`() {
+        val events = listOf(
+            invoked("inv1", stage),
+            wrote("inv1", "frontend/postcss.config.js", "h1"),
+            invoked("inv2", stage),
+            wrote("inv2", "frontend/postcss.config.cjs", "h1"),
+            invoked("inv3", stage),
+            deleted("inv3", "frontend/postcss.config.js"),
+        )
+        assertEquals(listOf("frontend/postcss.config.cjs"), stageWrittenPathsFrom(events, stage))
+    }
+
+    @Test
+    fun `a path deleted and then rewritten is back in the manifest`() {
+        val events = listOf(
+            invoked("inv1", stage),
+            wrote("inv1", "frontend/vite.config.ts", "h1"),
+            invoked("inv2", stage),
+            deleted("inv2", "frontend/vite.config.ts"),
+            invoked("inv3", stage),
+            wrote("inv3", "frontend/vite.config.ts", "h2"),
+        )
+        assertEquals(listOf("frontend/vite.config.ts"), stageWrittenPathsFrom(events, stage))
+    }
+
+    @Test
+    fun `surviving writes are unaffected by a sibling deletion`() {
+        val events = listOf(
+            invoked("inv1", stage),
+            wrote("inv1", "frontend/src/App.tsx", "h1"),
+            invoked("inv2", stage),
+            wrote("inv2", "frontend/tailwind.config.js", "h2"),
+            invoked("inv3", stage),
+            deleted("inv3", "frontend/src/App.css"),
+        )
+        assertEquals(
+            listOf("frontend/src/App.tsx", "frontend/tailwind.config.js"),
+            stageWrittenPathsFrom(events, stage),
+        )
+    }
+
+    @Test
+    fun `another stage's writes stay out of this stage's manifest`() {
+        val events = listOf(
+            invoked("inv1", stage),
+            wrote("inv1", "frontend/src/App.tsx", "h1"),
+            invoked("inv2", other),
+            wrote("inv2", "frontend/src/Sessions.tsx", "h2"),
+        )
+        assertEquals(listOf("frontend/src/App.tsx"), stageWrittenPathsFrom(events, stage))
+        assertTrue(stageWrittenPathsFrom(events, other) == listOf("frontend/src/Sessions.tsx"))
+    }
+
+    private var seq = 0L
+
+    private fun stored(payload: EventPayload): StoredEvent {
+        seq++
+        return StoredEvent(
+            metadata = EventMetadata(
+                eventId = EventId("e$seq"),
+                sessionId = SessionId("s1"),
+                timestamp = Instant.parse("2026-01-01T00:00:00Z"),
+                schemaVersion = 1,
+                causationId = null,
+                correlationId = null,
+            ),
+            sequence = seq,
+            sessionSequence = seq,
+            payload = payload,
+        )
+    }
+
+    private fun invoked(invocationId: String, stageId: StageId) = stored(
+        ToolInvocationRequestedEvent(
+            invocationId = ToolInvocationId(invocationId),
+            sessionId = SessionId("s1"),
+            stageId = stageId,
+            toolName = "file_write",
+            tier = Tier.T3,
+            request = ToolRequest(
+                invocationId = ToolInvocationId(invocationId),
+                sessionId = SessionId("s1"),
+                stageId = stageId,
+                toolName = "file_write",
+                parameters = emptyMap(),
+            ),
+        ),
+    )
+
+    private fun wrote(invocationId: String, path: String, hash: String) = stored(
+        FileWrittenEvent(
+            invocationId = ToolInvocationId(invocationId),
+            sessionId = SessionId("s1"),
+            path = path,
+            postImageHash = hash,
+            preExisted = false,
+            timestampMs = 1L,
+        ),
+    )
+
+    /** A deletion is a [FileWrittenEvent] with no post-image. */
+    private fun deleted(invocationId: String, path: String) = stored(
+        FileWrittenEvent(
+            invocationId = ToolInvocationId(invocationId),
+            sessionId = SessionId("s1"),
+            path = path,
+            postImageHash = null,
+            preExisted = true,
+            timestampMs = 1L,
+        ),
+    )
+}
@@ -201,20 +201,30 @@ class FileEditTool(
    }

    /**
-     * 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.
+     * Locate [target] in [content] ignoring each line's leading/trailing whitespace AND blank lines —
+     * small models reconstruct the target from memory, so an exact-string miss is almost always
+     * indentation drift or a dropped blank line, not a wrong edit. Comparing only the non-blank lines
+     * survives both: blank-line drift shifts every later index, so a positional walk over raw lines
+     * misses the whole block over one absent empty line (observed live on vite.config.ts, main.tsx,
+     * index.css). Returns the matched file-line range iff exactly one block matches; interior blank
+     * lines of the file fall inside the range and are consumed by the replacement, which is the
+     * intent — the caller sent a replacement for that whole block.
+     * ponytail: mixed tab/space inside a line still has to match after trim().
     */
    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] }
+        // (originalIndex, trimmed) for the file's non-blank lines only.
+        val fileNonBlank = fileLines.withIndex().filter { it.value.isNotBlank() }
+            .map { it.index to it.value.trim() }
+        val normTarget = target.split("\n").filter { it.isNotBlank() }.map { it.trim() }
+        if (normTarget.isEmpty() || normTarget.size > fileNonBlank.size) return null
+        val hits = (0..fileNonBlank.size - normTarget.size).filter { start ->
+            normTarget.indices.all { fileNonBlank[start + it].second == normTarget[it] }
        }
-        return if (starts.size == 1) starts[0] until (starts[0] + targetLines.size) else null
+        if (hits.size != 1) return null
+        val start = fileNonBlank[hits[0]].first
+        val end = fileNonBlank[hits[0] + normTarget.size - 1].first
+        return start..end
    }

    /** Rebase [replacement]'s indentation onto [baseIndent], preserving its own relative structure. */
@@ -216,6 +216,75 @@ class FileEditToolTest {
        )
    }

+    @Test
+    fun `replace tolerates a dropped blank line in the target`(): Unit = runBlocking {
+        // Verbatim from live session fced377e: the model reconstructed vite.config.ts from memory and
+        // omitted the blank line before the comment. A positional walk over raw lines shifts every
+        // later index and misses the whole block, so 4-of-6 file_edit calls failed on drift like this.
+        val tempDir = Files.createTempDirectory("file_edit_blankline")
+        val filePath = tempDir.resolve("vite.config.ts")
+        Files.writeString(
+            filePath,
+            "import { defineConfig } from 'vite'\n" +
+                "import react from '@vitejs/plugin-react'\n" +
+                "\n" + // the blank line the model dropped
+                "// https://vite.dev/config/\n" +
+                "export default defineConfig({\n" +
+                "  plugins: [react()],\n" +
+                "})\n",
+        )
+        val tool = FileEditTool(allowedPaths = setOf(tempDir))
+        val request = createRequest(
+            mapOf(
+                "operation" to "replace",
+                "path" to filePath.toString(),
+                "target" to "import { defineConfig } from 'vite'\n" +
+                    "import react from '@vitejs/plugin-react'\n" +
+                    "// https://vite.dev/config/\n" +
+                    "export default defineConfig({\n" +
+                    "  plugins: [react()],\n" +
+                    "})",
+                "replacement" to "import { defineConfig } from 'vite'\n" +
+                    "import react from '@vitejs/plugin-react'\n" +
+                    "export default defineConfig({\n" +
+                    "  plugins: [react()],\n" +
+                    "  server: { port: 5173 },\n" +
+                    "})",
+            ),
+        )
+
+        val result = tool.execute(request)
+        assertTrue(result is ToolResult.Success, "blank-line-drift replace should succeed")
+        assertTrue(
+            Files.readString(filePath).contains("server: { port: 5173 }"),
+            "the replacement should have been applied",
+        )
+    }
+
+    @Test
+    fun `replace still refuses a target that is ambiguous once blank lines are ignored`(): Unit = runBlocking {
+        // Ignoring blank lines must not turn a genuinely ambiguous edit into a silent wrong one.
+        val tempDir = Files.createTempDirectory("file_edit_blank_ambiguous")
+        val filePath = tempDir.resolve("dup.ts")
+        // Both sites are blank-separated, so there is no exact match to short-circuit on — the
+        // blank-line-insensitive walk is what has to reject this.
+        Files.writeString(filePath, "call()\n\nother()\n\ncall()\n\nother()\n")
+        val tool = FileEditTool(allowedPaths = setOf(tempDir))
+        val request = createRequest(
+            mapOf(
+                "operation" to "replace",
+                "path" to filePath.toString(),
+                "target" to "call()\nother()",
+                "replacement" to "call2()\nother2()",
+            ),
+        )
+
+        assertTrue(
+            tool.validateRequest(request) is ValidationResult.Invalid,
+            "two blank-line-normalized matches must stay ambiguous, not pick one",
+        )
+    }
+
    @Test
    fun `replace accepts content as an alias for replacement`(): Unit = runBlocking {
        val tempDir = Files.createTempDirectory("file_edit_alias")