From f08a78443294505b11644b6f7b077d9f7b7b5b26 Mon Sep 17 00:00:00 2001 From: kami Date: Tue, 21 Jul 2026 15:22:39 +0400 Subject: [PATCH] fix(orchestration): break the no-op-write loop + un-rot retry feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 " 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 --- .../builder/DefaultContextPackBuilder.kt | 4 ++- .../kernel/orchestration/ContextFeedback.kt | 4 ++- .../orchestration/SessionOrchestrator.kt | 18 +++++++--- .../SessionOrchestratorExecution.kt | 21 ++++++++---- .../SessionOrchestratorPreview.kt | 6 ++-- .../SessionOrchestratorWorkspace.kt | 15 ++++++++- .../core/transitions/graph/StageConfig.kt | 8 +++-- .../tools/SandboxedToolExecutor.kt | 29 +++++++++++++++- .../SandboxedToolExecutorFileMutationTest.kt | 33 +++++++++++++++++++ 9 files changed, 120 insertions(+), 18 deletions(-) diff --git a/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt b/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt index 4119863c..a50cabba 100644 --- a/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt +++ b/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt @@ -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 diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/ContextFeedback.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/ContextFeedback.kt index 6815e7c7..76c8e799 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/ContextFeedback.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/ContextFeedback.kt @@ -57,7 +57,9 @@ fun buildRetryFeedbackEntry(events: List, 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 " + diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestrator.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestrator.kt index 1e3c8162..28c1f6f4 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestrator.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestrator.kt @@ -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) diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorExecution.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorExecution.kt index fb7a36b2..94af0b58 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorExecution.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorExecution.kt @@ -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 } diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt index ec2bc5f4..2fc4c07d 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt @@ -283,8 +283,10 @@ internal fun renderDecomposePreview(parameters: Map): 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(")") } diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt index 1e14e253..9a1a6eef 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt @@ -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 } diff --git a/core/transitions/src/main/kotlin/com/correx/core/transitions/graph/StageConfig.kt b/core/transitions/src/main/kotlin/com/correx/core/transitions/graph/StageConfig.kt index 1ff834cb..7c503b35 100644 --- a/core/transitions/src/main/kotlin/com/correx/core/transitions/graph/StageConfig.kt +++ b/core/transitions/src/main/kotlin/com/correx/core/transitions/graph/StageConfig.kt @@ -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 = - setOf("file_read", "list_dir", "glob", "grep", "tool_output") + setOf("file_read", "list_dir", "glob", "grep") } } diff --git a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutor.kt b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutor.kt index 1abaf99b..494925f3 100644 --- a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutor.kt +++ b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutor.kt @@ -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, + preImages: Map, + ): 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( diff --git a/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutorFileMutationTest.kt b/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutorFileMutationTest.kt index e96a3c04..56f05195 100644 --- a/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutorFileMutationTest.kt +++ b/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/SandboxedToolExecutorFileMutationTest.kt @@ -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()