fix(orchestration): break the no-op-write loop + un-rot retry feedback

A stage (configure_ethos_theme) looped forever re-writing an identical postcss.config.js: file_write returned "written successfully" on a byte- identical write, and the retry-feedback entry that says WHY it failed got truncated out of context every turn, so the model cold-started on the same wrong idea. CAS hashes leaked into its face from two sides too. - SandboxedToolExecutor: a write whose result is byte-identical to disk now returns "No change: … nothing was written" (+ noop=true) instead of a false success signal. Uses the pre-image hashes already captured for reversibility. - DefaultContextPackBuilder: pin "retryFeedback" in neverDropSourceTypes so the failure reason + already-written-files list survives truncation. - ContextFeedback: drop the opaque "— CAS <hash>" from the retry entry; path only. - tool_output withheld until the session actually spills over-cap output to CAS (StageConfig.ALWAYS_AVAILABLE_READ_TOOLS -> on-demand via sessionHasSpilledOutput), so a hash-eating tool isn't advertised to every stage that never spills. - Wrap 10 long lines in kernel to bring detekt back under maxIssues (99 -> 89). Tests: infrastructure:tools (+2 no-op cases), core:{transitions,context,kernel} green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-21 15:22:39 +04:00
parent 8a60778ca7
commit f08a784432
9 changed files with 120 additions and 18 deletions
@@ -57,7 +57,9 @@ class DefaultContextPackBuilder(
    // "remainingDelta" is the shrinking stage-contract checklist (stage-termination design
    // 2026-07-11): it must survive every budget/dedup pass, since its entire purpose is to give
    // the model a progress signal that outlives the truncation which otherwise wipes its memory.
-    private val neverDropSourceTypes = setOf("steeringNote", "eventHistory", "factSheet", "remainingDelta")
+    // "retryFeedback" carries WHY the last attempt failed + the files already written this stage; if
+    // truncation evicts it the model cold-starts on the same wrong idea every turn (the write-loop rot).
+    private val neverDropSourceTypes = setOf("steeringNote", "eventHistory", "factSheet", "remainingDelta", "retryFeedback")

    private companion object {
        const val CHARS_PER_TOKEN = 4
@@ -57,7 +57,9 @@ fun buildRetryFeedbackEntry(events: List<StoredEvent>, stageId: StageId): Contex
                "Files you have already written this stage (authoritative current images — patch " +
                    "these, do NOT re-read to rediscover them):",
            )
-            currentImages.forEach { (path, hash) -> appendLine("- $path — CAS $hash") }
+            // Path only — the raw CAS hash is opaque noise the model can't act on and confuses it into
+            // reasoning about hashes; it patches by path via file_read/file_write.
+            currentImages.forEach { (path, _) -> appendLine("- $path") }
        }
        append(
            "Repair the recorded image and the named failure above first. Do not re-discover " +
@@ -349,10 +349,20 @@ abstract class SessionOrchestrator(
                // Read the log ONCE for the read-only check instead of once per tool inside the filter
                // (the flag is tool-independent) — this filter runs per tool per inference round.
                val readOnlyMode = isReadOnlyMode(sessionId)
-                stageConfig.effectiveAllowedTools
+                // tool_output is withheld until the session has actually spilled an over-cap output to
+                // CAS — only then can it retrieve anything, and only then is its hash-ref marker in play.
+                val toolNames = stageConfig.effectiveAllowedTools.let { declared ->
+                    if (declared.isNotEmpty() && sessionHasSpilledOutput(sessionId)) {
+                        declared + TOOL_OUTPUT_TOOL
+                    } else {
+                        declared
+                    }
+                }
+                toolNames
                    .mapNotNull { effectives.registry?.resolve(it) }
                    .filter { tool ->
-                        // ponytail: filter write tools while read-before-write block is active; restored once a read completes
+                        // ponytail: filter write tools while read-before-write block is active;
+                        // restored once a read completes
                        !readOnlyMode || ToolCapability.FILE_WRITE !in tool.requiredCapabilities
                    }
                    .filter { tool ->
@@ -371,8 +381,8 @@ abstract class SessionOrchestrator(
                    } + ToolDefinition(
                        function = ToolFunction(
                            name = STAGE_COMPLETE_TOOL,
-                            description = "Call this tool when the stage's goal is fully met and no further tool calls are needed. " +
-                                    "The orchestrator will proceed to the next stage.",
+                            description = "Call this tool when the stage's goal is fully met and no " +
+                                    "further tool calls are needed. The orchestrator will proceed to the next stage.",
                            parameters = JsonObject(emptyMap()),
                        ),
                    ) + emitArtifactTool(stageConfig)
@@ -277,11 +277,12 @@ internal suspend fun SessionOrchestrator.executeStage(
        ?.let { buildRemainingDeltaEntry(contractFailureItems(it)) }
        ?.let { listOf(it) } ?: emptyList()
    var accumulatedEntries = stampBuckets(
-        systemPrompt + operatingGuidance + promotedConcepts + successfulPlanShapes + verifiedBaseline + intentEntries + profileEntries + projectProfileEntries + agentInstructionsEntries +
+        systemPrompt + operatingGuidance + promotedConcepts + successfulPlanShapes + verifiedBaseline +
+            intentEntries + profileEntries + projectProfileEntries + agentInstructionsEntries +
            journalEntries + repoMapEntries + claimedTaskEntries +
            needsEntries + schemaEntries + vocabularyEntries + promptEntries + steeringEntries +
-            rejectionEntries + clarificationEntries + retryFeedbackEntries + groundingFeedbackEntries + recoveryTicketEntries +
-            remainingDeltaEntries,
+            rejectionEntries + clarificationEntries + retryFeedbackEntries + groundingFeedbackEntries +
+            recoveryTicketEntries + remainingDeltaEntries,
    )
    val contextPack = runCatching {
        contextPackBuilder.build(
@@ -397,7 +398,9 @@ internal suspend fun SessionOrchestrator.executeStage(
        val emitCall = inferenceResult.response.toolCalls.firstOrNull { it.function.name == EMIT_ARTIFACT_TOOL }
        if (emitCall != null && llmEmittedSlots.isNotEmpty()) {
            val emitSlot = llmEmittedSlots.first()
-            when (val res = artifactExtractionPipeline.run(emitCall.function.arguments, emitSlot.kind.deriveJsonSchema())) {
+            when (
+                val res = artifactExtractionPipeline.run(emitCall.function.arguments, emitSlot.kind.deriveJsonSchema())
+            ) {
                is ArtifactExtractionPipeline.ExtractionResult.Resolved -> {
                    llmArtifactOverride = res.canonicalJson.toString()
                    break
@@ -597,13 +600,19 @@ internal suspend fun SessionOrchestrator.executeStage(
                when (val res = artifactExtractionPipeline.run(rawArtifactText, slot.kind.deriveJsonSchema())) {
                    is ArtifactExtractionPipeline.ExtractionResult.Resolved -> {
                        if (res.repaired) {
-                            emitArtifactRepairAttempted(sessionId, stageId, slot, ArtifactFailure.FORMATTING, "DETERMINISTIC")
+                            emitArtifactRepairAttempted(
+                                sessionId, stageId, slot, ArtifactFailure.FORMATTING, "DETERMINISTIC",
+                            )
                            emitArtifactRepairResolved(sessionId, stageId, slot, res.canonicalJson.toString())
                        }
                        res.canonicalJson.toString()
                    }
                    is ArtifactExtractionPipeline.ExtractionResult.Unresolved ->
-                        when (val ladder = repairArtifact(sessionId, stageId, slot, res, stageConfig, effectives, config.stageTimeoutMs)) {
+                        when (
+                            val ladder = repairArtifact(
+                                sessionId, stageId, slot, res, stageConfig, effectives, config.stageTimeoutMs,
+                            )
+                        ) {
                            is ArtifactLadderOutcome.Text -> ladder.text
                            is ArtifactLadderOutcome.Reject -> return ladder.failure
                        }
@@ -283,8 +283,10 @@ internal fun renderDecomposePreview(parameters: Map<String, Any>): String? {
        parentTitle?.let { append("\n  epic: ").append(it) }
        tasks.forEachIndexed { i, e ->
            val o = e as? JsonObject
-            append("\n  ").append(i + 1).append(". ").append((o?.get("title") as? JsonPrimitive)?.content ?: "(untitled)")
-            val afters = ((o?.get("depends_on") as? JsonArray)?.mapNotNull { (it as? JsonPrimitive)?.content } ?: emptyList())
+            append("\n  ").append(i + 1).append(". ")
+                .append((o?.get("title") as? JsonPrimitive)?.content ?: "(untitled)")
+            val afters = ((o?.get("depends_on") as? JsonArray)
+                ?.mapNotNull { (it as? JsonPrimitive)?.content } ?: emptyList())
                .map { d -> (refToIndex[d] ?: d.toIntOrNull())?.let { titleAt(it) } ?: d }
            if (afters.isNotEmpty()) append("  (after: ").append(afters.joinToString(", ")).append(")")
        }
@@ -163,7 +163,20 @@ internal fun SessionOrchestrator.isReadOnlyMode(sessionId: SessionId): Boolean {
    return blocked
 }

-internal fun SessionOrchestrator.mandateSuppressedByTicket(sessionId: SessionId, stageId: StageId, stageConfig: StageConfig): Boolean {
+/** True once any tool result in the session has spilled its full output to CAS (recorded as a
+ *  non-null [ToolReceipt.fullOutputHash]). Until then the retrieval tool `tool_output` is withheld
+ *  from stage tool lists — it can't retrieve anything before a spill, and an unusable hash-eating tool
+ *  in every request just nudges models into reasoning about opaque hashes. */
+internal fun SessionOrchestrator.sessionHasSpilledOutput(sessionId: SessionId): Boolean =
+    eventStore.read(sessionId).any {
+        (it.payload as? ToolExecutionCompletedEvent)?.receipt?.fullOutputHash != null
+    }
+
+internal fun SessionOrchestrator.mandateSuppressedByTicket(
+    sessionId: SessionId,
+    stageId: StageId,
+    stageConfig: StageConfig,
+): Boolean {
    if (stageConfig.metadata["role"] == "recovery") return false
    return eventStore.read(sessionId)
        .mapNotNull { it.payload as? TransitionExecutedEvent }
@@ -73,8 +73,12 @@ data class StageConfig(
        get() = if (allowedTools.isEmpty()) allowedTools else allowedTools + ALWAYS_AVAILABLE_READ_TOOLS

    companion object {
-        /** Read-only tools every tool-granting stage may call regardless of its declared set. */
+        /** Read-only tools every tool-granting stage may call regardless of its declared set.
+         *  `tool_output` is deliberately NOT here — it can only retrieve output that has actually been
+         *  spilled to CAS, so the orchestrator adds it on demand (once a spill has occurred) rather than
+         *  advertising a hash-eating tool to every stage that will never spill (context noise that nudges
+         *  models into reasoning about opaque hashes). */
        val ALWAYS_AVAILABLE_READ_TOOLS: Set<String> =
-            setOf("file_read", "list_dir", "glob", "grep", "tool_output")
+            setOf("file_read", "list_dir", "glob", "grep")
    }
 }
@@ -95,7 +95,7 @@ class SandboxedToolExecutor(
                // reconstruct: pre/post-image hashes (reversibility) and research-source markers.
                emitResearchSourceEvents(sessionId, request.stageId, result)
                emitFileMutations(sessionId, invocationId, affectedPaths, preImages)
-                result
+                reframeIfNoOpWrite(result, affectedPaths, preImages)
            }

            is ToolResult.Failure -> {
@@ -158,6 +158,33 @@ class SandboxedToolExecutor(
        }
    }

+    /**
+     * A write whose result is byte-identical to what was already on disk is a no-op, but the delegate
+     * still reports "written successfully" — a false progress signal that traps looping agents (they
+     * re-write the same content, see success, and never learn nothing changed). Rewrite the receipt to
+     * the truth. Only fires when EVERY affected path pre-existed with the same content, so a partial
+     * change (one file touched, one identical) is still reported as a real write.
+     * ponytail: hash-compares via the CAS, so only active when artifactStore is wired; else pass-through.
+     */
+    private suspend fun reframeIfNoOpWrite(
+        result: ToolResult.Success,
+        affectedPaths: Set<Path>,
+        preImages: Map<Path, PreImage>,
+    ): ToolResult.Success {
+        if (artifactStore == null || affectedPaths.isEmpty()) return result
+        val unchanged = affectedPaths.all { path ->
+            val pre = preImages[path]
+            pre?.existed == true && pre.hash != null && pre.hash == storeBytes(path)
+        }
+        if (!unchanged) return result
+        val paths = affectedPaths.joinToString(", ") { it.toString() }
+        return result.copy(
+            output = "No change: $paths already contained this exact content; nothing was written. " +
+                "Do not repeat this write — make a different change or complete the stage.",
+            metadata = result.metadata + ("noop" to "true"),
+        )
+    }
+
    // --- event emission ---

    private suspend fun emitStarted(
@@ -115,6 +115,39 @@ class SandboxedToolExecutorFileMutationTest {
        assertEquals("HELLO", store.get(ArtifactId(fw.postImageHash!!))!!.toString(Charsets.UTF_8))
    }

+    @Test
+    fun `writing identical content reframes the success message as a no-op`(): Unit = runBlocking {
+        val dir = Files.createTempDirectory("sbx-noop").toRealPath()
+        val target = dir.resolve("f.txt")
+        Files.writeString(target, "SAME")
+        val tool = FileWriteTool(allowedPaths = setOf(dir))
+        val store = FakeArtifactStore()
+        val events = CapturingEventStore()
+
+        val result = executor(tool, store, events).execute(writeRequest(target.toString(), "SAME"))
+
+        assertTrue(result is ToolResult.Success)
+        val success = result as ToolResult.Success
+        assertTrue(success.output.startsWith("No change:"), success.output)
+        assertEquals("true", success.metadata["noop"])
+    }
+
+    @Test
+    fun `changing content keeps the normal success message`(): Unit = runBlocking {
+        val dir = Files.createTempDirectory("sbx-changed").toRealPath()
+        val target = dir.resolve("f.txt")
+        Files.writeString(target, "OLD")
+        val tool = FileWriteTool(allowedPaths = setOf(dir))
+        val store = FakeArtifactStore()
+        val events = CapturingEventStore()
+
+        val result = executor(tool, store, events).execute(writeRequest(target.toString(), "NEW"))
+
+        val success = result as ToolResult.Success
+        assertTrue(!success.output.startsWith("No change:"), success.output)
+        assertNull(success.metadata["noop"])
+    }
+
    @Test
    fun `no artifact store means no FileWrittenEvent (backward compatible)`(): Unit = runBlocking {
        val dir = Files.createTempDirectory("sbx-nostore").toRealPath()