diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/Main.kt b/apps/server/src/main/kotlin/com/correx/apps/server/Main.kt index b3b7736b..3b4707c4 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/Main.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/Main.kt @@ -247,6 +247,10 @@ fun main() { val defaultOrchestrationConfig = OrchestrationConfig( sandboxRoot = sandboxRoot, defaultSystemPromptPath = toolsConfig.defaultSystemPromptPath, + // Local models are slow on large implementer/reviewer turns (plan + design + file + // contents); 60s was timing out ~mid-run and burning every retry. 3 min gives the + // turn room to complete. TODO: surface this as a config knob instead of a hardcode. + stageTimeoutMs = 180_000L, ) val journalCompactionService = JournalCompactionService( artifactStore = artifactStore, diff --git a/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenKind.kt b/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenKind.kt index a5efdf0c..32bb389c 100644 --- a/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenKind.kt +++ b/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenKind.kt @@ -10,6 +10,7 @@ object FileWrittenKind : ArtifactKind { "path" to JsonSchemaProperty(type = "string", description = "Relative file path, no .. components"), "content" to JsonSchemaProperty(type = "string", description = "File content"), "mode" to JsonSchemaProperty(type = "string", description = "Unix file mode", default = "0644"), + "diff" to JsonSchemaProperty(type = "string", description = "Unified diff of the change"), ), required = listOf("path", "content"), additionalProperties = false, diff --git a/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenPayload.kt b/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenPayload.kt index d0e4e3de..9ccabd07 100644 --- a/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenPayload.kt +++ b/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/FileWrittenPayload.kt @@ -15,4 +15,7 @@ data class FileWrittenArtifact( val contentHash: String, val size: Long, val mode: String, + // Unified diff of the change, so downstream consumers (e.g. the reviewer stage) can see WHAT + // changed rather than only metadata. Null/absent when no diff was captured. + val diff: String? = null, ) 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 2ecef17f..a25b4136 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 @@ -12,7 +12,9 @@ import com.correx.core.approvals.model.ApprovalScopeIdentity import com.correx.core.approvals.model.DomainApprovalRequest import com.correx.core.artifacts.ArtifactState import com.correx.core.artifacts.kind.JsonSchema +import com.correx.core.artifacts.kind.FileWrittenArtifact import com.correx.core.artifacts.kind.ProcessResultArtifact +import com.correx.core.artifacts.kind.TypedArtifactSlot import com.correx.core.artifactstore.ArtifactStore import com.correx.core.context.builder.ContextPackBuilder import com.correx.core.context.model.ContextEntry @@ -40,9 +42,13 @@ import com.correx.core.events.events.OrchestrationPausedEvent import com.correx.core.events.events.OrchestrationResumedEvent import com.correx.core.events.events.RiskAssessedEvent import com.correx.core.events.events.ExecutionPlanLockedEvent +import com.correx.core.events.events.FileWrittenEvent import com.correx.core.events.events.SteeringNoteAddedEvent +import com.correx.core.events.events.ToolExecutionCompletedEvent +import com.correx.core.events.events.ToolExecutionFailedEvent import com.correx.core.events.events.ToolExecutionRejectedEvent import com.correx.core.events.events.ToolInvocationRequestedEvent +import com.correx.core.events.events.ToolReceipt import com.correx.core.events.events.ToolRequest import com.correx.core.events.risk.RiskAction import com.correx.core.events.risk.RiskSummary @@ -87,6 +93,7 @@ import com.correx.core.risk.RiskContext import com.correx.core.risk.toApprovalTier import com.correx.core.sessions.Session import com.correx.core.tools.compression.ToolOutputContext +import com.correx.core.tools.contract.FileAffectingTool import com.correx.core.tools.contract.Tool import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -114,6 +121,8 @@ import com.correx.core.journal.DecisionJournalRenderer import com.correx.core.journal.DefaultDecisionJournalRepository import com.correx.core.kernel.orchestration.subagent.SubagentRunner import org.slf4j.LoggerFactory +import java.nio.file.Files +import java.nio.file.Paths import java.util.* import java.util.concurrent.* import java.util.concurrent.atomic.* @@ -121,6 +130,7 @@ import kotlin.coroutines.cancellation.CancellationException private const val MAX_TOOL_ROUNDS = 10 private const val STAGE_COMPLETE_TOOL = "stage_complete" +private const val OUTPUT_SUMMARY_LIMIT = 500 private const val REPO_MAP_INJECT_TOP_K = 30 @SuppressWarnings( @@ -366,16 +376,74 @@ abstract class SessionOrchestrator( sessionId, stageId, currentContext, stageConfig, config.stageTimeoutMs, responseFormat, effectives, ) var toolRounds = 0 + + val fileWrittenSlots = stageConfig.produces.filter { it.kind.id == "file_written" } + fun owesFileWrite(): Boolean = fileWrittenSlots.isNotEmpty() && + fileWrittenSlots.any { artifactContentCache["${sessionId.value}:${it.name.value}"].isNullOrBlank() } + + // Append a corrective tool-result and re-run inference (bounded by MAX_TOOL_ROUNDS). + // Returns the new result rather than mutating inferenceResult, to preserve smart casts. + suspend fun pushBack(nudge: String): InferenceResult { + accumulatedEntries = accumulatedEntries + ContextEntry( + id = ContextEntryId(UUID.randomUUID().toString()), + layer = ContextLayer.L2, + sourceType = "toolResult", + sourceId = UUID.randomUUID().toString(), + content = nudge, + tokenEstimate = estimateTokens(nudge), + role = EntryRole.TOOL, + ) + currentContext = contextPackBuilder.build( + id = ContextPackId(UUID.randomUUID().toString()), + sessionId = sessionId, + stageId = stageId, + entries = accumulatedEntries, + budget = TokenBudget(limit = stageConfig.tokenBudget), + ) + emitContextTruncationIfNeeded(sessionId, stageId, currentContext) + toolRounds++ + return runInference( + sessionId, stageId, currentContext, stageConfig, config.stageTimeoutMs, responseFormat, effectives, + ) + } + + val writeNudge = "ERROR: you described the change but did not apply it. You MUST call the " + + "file_edit or file_write tool to write the change to the file before finishing — do " + + "not output the file contents as a message." + while ( inferenceResult is InferenceResult.Success && - inferenceResult.response.finishReason is FinishReason.ToolCall && - toolRounds < MAX_TOOL_ROUNDS + toolRounds < MAX_TOOL_ROUNDS && + (inferenceResult.response.finishReason is FinishReason.ToolCall || owesFileWrite()) ) { + // Content turn (no tool call) but the stage still owes a file_written artifact: the + // model emitted the change as prose instead of invoking the write tool. Nudge it to + // actually write (F-018). + if (inferenceResult.response.finishReason !is FinishReason.ToolCall) { + inferenceResult = pushBack(writeNudge) + continue + } // stage_complete is a meta-tool: the LLM calls it to signal the stage's goal is met. // Skip tool dispatch — the loop exits and normal validation/transition follows // (emitToolArtifacts + verifyProduces run on the success path after the loop). if (inferenceResult.response.toolCalls.any { it.function.name == STAGE_COMPLETE_TOOL }) { - break + // Premature-completion guard: don't accept stage_complete while the stage still + // owes an artifact. For an LLM-emitted slot, nudge to emit the JSON (F-010); for a + // file_written slot, nudge to invoke the write tool (F-018). Otherwise complete. + val owedLlm = llmEmittedSlots.firstOrNull()?.takeIf { + inferenceResult.response.text.isBlank() && + artifactContentCache["${sessionId.value}:${it.name.value}"].isNullOrBlank() + } + when { + owedLlm != null -> inferenceResult = pushBack( + "ERROR: stage_complete is not allowed yet — you have not emitted the " + + "required '${owedLlm.name.value}' artifact. Respond with the artifact as " + + "a single JSON message (no tool call) matching the provided schema.", + ) + owesFileWrite() -> inferenceResult = pushBack(writeNudge) + else -> break + } + continue } val toolEntries = dispatchToolCalls(sessionId, stageId, inferenceResult.response.toolCalls, stageConfig, effectives) @@ -387,14 +455,9 @@ abstract class SessionOrchestrator( retryable = false, ) } - val errorEntry = toolEntries.firstOrNull { it.content.startsWith("ERROR:") } - if (errorEntry != null) { - emitProcessResultEvents(sessionId, stageId, stageConfig) - return StageExecutionResult.Failure( - errorEntry.content.removePrefix("ERROR: "), - retryable = true, - ) - } + // Recoverable (ERROR:) tool failures are NOT terminal — they flow back into the + // loop as tool-result context so the model can see the error and adapt (bounded by + // MAX_TOOL_ROUNDS). Only FATAL: failures (handled above) abort the stage. accumulatedEntries = accumulatedEntries + toolEntries currentContext = contextPackBuilder.build( id = ContextPackId(UUID.randomUUID().toString()), @@ -459,6 +522,21 @@ abstract class SessionOrchestrator( ): String = "[SessionOrchestrator] stage=${stageId.value}: " + "failed to load prompt '$path': ${throwable.message}" + /** + * Strip an outer markdown code fence (```` ```json `` / ``` ``` ````) when the whole response + * is a single fenced block. Conservative: only acts when the trimmed text both starts and ends + * with a fence, so prose responses that merely contain an inline block are left untouched. + */ + private fun stripCodeFence(text: String): String { + val trimmed = text.trim() + if (!trimmed.startsWith("```") || !trimmed.endsWith("```")) return text + val withoutClose = trimmed.removeSuffix("```").trimEnd() + val firstNewline = withoutClose.indexOf('\n') + if (firstNewline < 0) return text + // Drop the opening fence line (``` optionally followed by a language tag). + return withoutClose.substring(firstNewline + 1).trim() + } + @Suppress("CyclomaticComplexMethod") private suspend fun dispatchToolCalls( sessionId: SessionId, @@ -469,6 +547,7 @@ abstract class SessionOrchestrator( ): List { val executor = effectives.executor ?: return emptyList() val processResultSlots = stageConfig.produces.filter { it.kind.id == "process_result" } + val fileWrittenSlots = stageConfig.produces.filter { it.kind.id == "file_written" } return toolCalls.flatMap { toolCall -> val invocationId = ToolInvocationId(UUID.randomUUID().toString()) val parameters = runCatching { @@ -730,6 +809,16 @@ abstract class SessionOrchestrator( ) } + // Record the tool execution outcome as events (invariant #5: every side effect is + // captured; silent execution is not allowed). For file-mutating tools, materialise a + // real file_written artifact from the actual on-disk result — so a stage that did NOT + // write cannot have its file_written slot rubber-stamped as validated (F-015), and the + // artifact carries the diff as evidence of the change. + recordToolExecution( + sessionId, stageId, toolCall, invocationId, tier, result, + tool as? FileAffectingTool, request, fileWrittenSlots, + ) + val sourceId = toolCall.id ?: invocationId.value val assistantEntry = ContextEntry( id = ContextEntryId(UUID.randomUUID().toString()), @@ -942,12 +1031,120 @@ abstract class SessionOrchestrator( stageConfig.produces .filter { !it.kind.llmEmitted } .forEach { slot -> + // A file_written slot must be backed by real content materialised from an actual + // successful write (see recordToolExecution). If none happened, do NOT fabricate a + // validated artifact (F-015) — leave the slot unproduced so verifyProduces fails the + // stage and it retries, rather than reporting a green run that changed nothing. + if (slot.kind.id == "file_written" && + artifactContentCache["${sessionId.value}:${slot.name.value}"] == null + ) { + return@forEach + } emit(sessionId, ArtifactCreatedEvent(slot.name, sessionId, stageId, schemaVersion = 1)) emit(sessionId, ArtifactValidatingEvent(slot.name, sessionId, stageId)) emit(sessionId, ArtifactValidatedEvent(slot.name, sessionId, stageId)) } } + private suspend fun recordToolExecution( + sessionId: SessionId, + stageId: StageId, + toolCall: ToolCallRequest, + invocationId: ToolInvocationId, + tier: Tier, + result: ToolResult, + fileTool: FileAffectingTool?, + request: ToolRequest, + fileWrittenSlots: List, + ) { + when (result) { + is ToolResult.Failure -> emit( + sessionId, + ToolExecutionFailedEvent(invocationId, sessionId, toolCall.function.name, result.reason), + ) + + is ToolResult.Success -> { + val diff = result.metadata["diff"]?.takeIf { it.isNotBlank() } + val affected = fileTool?.affectedPaths(request).orEmpty() + emit( + sessionId, + ToolExecutionCompletedEvent( + invocationId = invocationId, + sessionId = sessionId, + toolName = toolCall.function.name, + receipt = ToolReceipt( + invocationId = invocationId, + toolName = toolCall.function.name, + exitCode = result.exitCode, + outputSummary = result.output.take(OUTPUT_SUMMARY_LIMIT), + affectedEntities = affected.map { it.toString() }, + durationMs = 0, + tier = tier, + timestamp = Clock.System.now(), + diff = diff, + ), + ), + ) + if (affected.isNotEmpty()) { + materializeFileWritten(sessionId, stageId, request, affected, fileWrittenSlots, diff) + affected.forEach { p -> + emit( + sessionId, + FileWrittenEvent( + invocationId = invocationId, + sessionId = sessionId, + path = p.toString(), + preExisted = true, + timestampMs = Clock.System.now().toEpochMilliseconds(), + ), + ) + } + } + } + } + } + + /** + * Materialise a [FileWrittenArtifact] from the actual on-disk file after a successful mutation, + * store it as the stage's file_written slot content, and record [ArtifactContentStoredEvent]. + * This ties the file_written artifact to a real write so validation can no longer be faked. + */ + private suspend fun materializeFileWritten( + sessionId: SessionId, + stageId: StageId, + request: ToolRequest, + affected: Set, + fileWrittenSlots: List, + diff: String?, + ) { + if (fileWrittenSlots.isEmpty()) return + val relPath = request.parameters["path"] as? String ?: return + val onDisk = affected.firstOrNull() ?: Paths.get(relPath) + val bytes = runCatching { Files.readAllBytes(onDisk) }.getOrNull() ?: return + val contentHash = artifactStore.put(bytes) + val artifact = FileWrittenArtifact( + path = relPath, + contentHash = contentHash.value, + size = bytes.size.toLong(), + mode = "0644", + diff = diff, + ) + val json = Json.encodeToString(FileWrittenArtifact.serializer(), artifact) + val storedHash = artifactStore.put(json.toByteArray()) + fileWrittenSlots.forEach { slot -> + artifactContentCache["${sessionId.value}:${slot.name.value}"] = json + emit( + sessionId, + ArtifactContentStoredEvent( + artifactId = slot.name, + contentHash = storedHash, + sessionId = sessionId, + stageId = stageId, + ), + ) + } + } + private suspend fun emitLlmArtifacts( sessionId: SessionId, stageId: StageId, @@ -1075,7 +1272,12 @@ abstract class SessionOrchestrator( if (isCancelled(sessionId)) return InferenceResult.Cancelled return try { - val response = withTimeout(timeoutMs) { provider.infer(request) } + val rawResponse = withTimeout(timeoutMs) { provider.infer(request) } + // Models frequently wrap a JSON artifact in a ```json … ``` markdown fence despite + // being told not to. The fence makes the artifact unparseable → validation fails. + // Strip an outer fence wrapper so the stored artifact, its CAS hash, the content + // cache, and validation all see the same clean payload. + val response = rawResponse.copy(text = stripCodeFence(rawResponse.text)) log.debug( "[Orchestrator] inference done session={} stage={} tokens={} latencyMs={}", sessionId.value, stageId.value, response.tokensUsed, response.latencyMs, diff --git a/core/validation/src/main/kotlin/com/correx/core/validation/pipeline/ValidationPipeline.kt b/core/validation/src/main/kotlin/com/correx/core/validation/pipeline/ValidationPipeline.kt index 45b2e147..e0926969 100644 --- a/core/validation/src/main/kotlin/com/correx/core/validation/pipeline/ValidationPipeline.kt +++ b/core/validation/src/main/kotlin/com/correx/core/validation/pipeline/ValidationPipeline.kt @@ -6,6 +6,8 @@ import com.correx.core.validation.model.ValidationReport import com.correx.core.validation.model.ValidationSection import com.correx.core.validation.model.ValidationSeverity +private const val ARTIFACT_PAYLOAD_SECTION = "artifact_payload" + class ValidationPipeline( private val validators: List, private val approvalTrigger: ApprovalTrigger? = null @@ -17,8 +19,12 @@ class ValidationPipeline( val section = validator.validate(context) sections += section if (section.issues.any { it.severity == ValidationSeverity.ERROR }) { - // Structural validation errors are not retryable — the graph must be fixed. - return ValidationOutcome.Rejected(ValidationReport(sections), retryable = false) + // Distinguish failure classes: a malformed/schema-invalid LLM artifact is + // retryable (the model is nondeterministic — a fresh attempt usually succeeds, + // up to the stage's max_retries; this is the invariant #7 "reject + retry" + // contract). Structural/graph errors are not retryable — the graph must be fixed. + val retryable = section.name == ARTIFACT_PAYLOAD_SECTION + return ValidationOutcome.Rejected(ValidationReport(sections), retryable = retryable) } } val report = ValidationReport(sections) diff --git a/docs/schemas/analysis.json b/docs/schemas/analysis.json index 47f63184..4c8e5fe9 100644 --- a/docs/schemas/analysis.json +++ b/docs/schemas/analysis.json @@ -6,12 +6,12 @@ "description": "what the request is asking for, in your own words" }, "requirements": { - "type": "string", - "description": "concrete requirements / acceptance criteria, newline-separated" + "type": "array", + "description": "concrete requirements / acceptance criteria, one per item" }, "affected_areas": { - "type": "string", - "description": "files, modules, or subsystems likely involved, newline-separated" + "type": "array", + "description": "files, modules, or subsystems likely involved, one per item" } }, "required": ["summary", "requirements"], diff --git a/docs/schemas/design.json b/docs/schemas/design.json index c557f455..b6184532 100644 --- a/docs/schemas/design.json +++ b/docs/schemas/design.json @@ -6,12 +6,12 @@ "description": "the chosen approach and why" }, "components": { - "type": "string", - "description": "components/files to add or change, newline-separated" + "type": "array", + "description": "components/files to add or change, one per item" }, "risks": { - "type": "string", - "description": "risks, trade-offs, or open questions, newline-separated" + "type": "array", + "description": "risks, trade-offs, or open questions, one per item" } }, "required": ["approach", "components"], diff --git a/docs/schemas/impl_plan.json b/docs/schemas/impl_plan.json index ec5e6161..d806bbe1 100644 --- a/docs/schemas/impl_plan.json +++ b/docs/schemas/impl_plan.json @@ -2,12 +2,12 @@ "type": "object", "properties": { "steps": { - "type": "string", - "description": "ordered, individually verifiable implementation steps, newline-separated" + "type": "array", + "description": "ordered, individually verifiable implementation steps, one per item" }, "verification": { - "type": "string", - "description": "how completion is checked (commands, tests), newline-separated" + "type": "array", + "description": "how completion is checked (commands, tests), one per item" } }, "required": ["steps"], diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/DiffUtil.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/DiffUtil.kt new file mode 100644 index 00000000..5465ff69 --- /dev/null +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/DiffUtil.kt @@ -0,0 +1,36 @@ +package com.correx.infrastructure.tools.filesystem + +import java.io.IOException +import java.nio.file.Files + +/** + * Produce a `diff -u` unified diff of a file mutation (labelled `a/path` → `b/path`), so the + * change is auditable downstream — the reviewer stage and the operator can see WHAT changed, not + * just that something did. Returns "" when there is no change. Best-effort: a diff that cannot be + * produced is non-fatal. + */ +fun unifiedDiff(pathString: String, original: String, updated: String): String { + if (original == updated) return "" + val tmpDir = Files.createTempDirectory("correx-diff") + val a = tmpDir.resolve("a") + val b = tmpDir.resolve("b") + return try { + Files.write(a, original.toByteArray()) + Files.write(b, updated.toByteArray()) + val process = ProcessBuilder( + "diff", "-u", "--label", "a/$pathString", "--label", "b/$pathString", + a.toString(), b.toString(), + ).redirectErrorStream(true).start() + val out = process.inputStream.bufferedReader().use { it.readText() } + process.waitFor() + out.trim() + } catch (e: IOException) { + "(diff unavailable: ${e.message})" + } finally { + runCatching { + Files.deleteIfExists(a) + Files.deleteIfExists(b) + Files.deleteIfExists(tmpDir) + } + } +} 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 3e726cbf..46e125bd 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 @@ -43,7 +43,9 @@ class FileEditTool( } override val name: String = "file_edit" - override val description: String = "Edit content in a file at the specified path" + override val description: String = "Edit a file. Prefer operation 'replace' (exact string " + + "swap) for targeted edits; use file_write to replace the whole file. Use 'patch' only " + + "when you can produce a valid unified diff that applies cleanly with 'patch -p1'." override val parametersSchema: JsonObject = buildJsonObject { put("type", "object") putJsonObject("properties") { @@ -53,19 +55,24 @@ class FileEditTool( } putJsonObject("operation") { put("type", "string") - put("description", "Either 'append', 'replace', or 'patch'") + put("description", "'replace' (recommended: exact string swap via target/" + + "replacement), 'append', or 'patch' (a unified diff; only if it applies cleanly)") } putJsonObject("content") { put("type", "string") - put("description", "Content to write. Used for 'append' and 'replace'.") + put("description", "Content to append. Required for 'append'.") } - putJsonObject("old_str") { + putJsonObject("target") { put("type", "string") - put("description", "Exact string to find. Required for 'patch'.") + put("description", "Exact, unique string to find. Required for 'replace'.") } - putJsonObject("new_str") { + putJsonObject("replacement") { put("type", "string") - put("description", "Replacement string. Required for 'patch'.") + put("description", "String to substitute for 'target'. Required for 'replace'.") + } + putJsonObject("patch") { + put("type", "string") + put("description", "A unified diff (patch -p1 format) to apply. Required for 'patch'.") } } put("required", buildJsonArray { @@ -149,22 +156,39 @@ class FileEditTool( override suspend fun execute(request: ToolRequest): ToolResult = withContext(Dispatchers.IO) { val validation = validateRequest(request) if (validation is ValidationResult.Invalid) { + // Recoverable: malformed/missing params are model-correctable — feed the error back so + // the model can fix the call (e.g. switch operation or supply the right argument) + // instead of aborting the whole stage. return@withContext ToolResult.Failure( invocationId = request.invocationId, reason = validation.reason, - recoverable = false, + recoverable = true, ) } val operation = request.parameters["operation"] as String val pathString = request.parameters["path"] as String val path = resolvePath(pathString) + val originalContent = runCatching { Files.readString(path) }.getOrDefault("") - runCatching { + val result = runCatching { performOperation(operation, path, pathString, request) }.getOrElse { e -> handleExecutionException(e, request.invocationId, pathString) } + + // Attach a unified diff of the actual change so the side effect is auditable downstream + // (the file_written artifact and its reviewer/operator surface carry real evidence). + if (result is ToolResult.Success) { + val updatedContent = runCatching { Files.readString(path) }.getOrDefault(originalContent) + val diff = unifiedDiff(pathString, originalContent, updatedContent) + result.copy( + output = if (diff.isBlank()) result.output else "${result.output}\n\n$diff", + metadata = result.metadata + ("diff" to diff), + ) + } else { + result + } } private fun performOperation( @@ -179,7 +203,7 @@ class FileEditTool( else -> ToolResult.Failure( invocationId = request.invocationId, reason = "Unknown operation: $operation", - recoverable = false, + recoverable = true, ) } @@ -213,7 +237,8 @@ class FileEditTool( } else { "Target '$target' found $occurrences times, expected exactly once" }, - recoverable = false, + // Recoverable: the model can adjust the target string and retry. + recoverable = true, ) } } @@ -239,10 +264,13 @@ class FileEditTool( output = "Patch applied successfully to $pathString: $output", ) } else { + // Recoverable: a non-applying patch is model-correctable — feed the error back so + // the model can fix the diff or fall back to 'replace'/file_write, not abort. ToolResult.Failure( invocationId = request.invocationId, - reason = "Patch failed with exit code $exitCode: $output", - recoverable = false, + reason = "Patch failed with exit code $exitCode: $output. " + + "Consider using operation 'replace' (exact string swap) or file_write instead.", + recoverable = true, ) } } finally { diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt index 269ca94f..61561b90 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt @@ -115,10 +115,12 @@ class FileReadTool( val endLine = request.parameters["end_line"]?.toString()?.toIntOrNull() if (!Files.exists(path)) + // Recoverable: the model often probes for files that may not exist (e.g. a + // speculative __init__.py). Feed the miss back so it can adapt, don't kill the stage. return@withContext ToolResult.Failure( invocationId = request.invocationId, reason = "File not found: $pathString", - recoverable = false, + recoverable = true, ) runCatching { diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt index 21566bf8..ccc9932b 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt @@ -142,12 +142,27 @@ class FileWriteTool( val operation = request.parameters["operation"] as String val pathString = request.parameters["path"] as String val path = resolvePath(pathString) + val originalContent = runCatching { Files.readString(path) }.getOrDefault("") - runCatching { + val result = runCatching { performOperation(operation, path, pathString, request) }.getOrElse { e -> handleExecutionException(e, request.invocationId, pathString) } + + // Attach a unified diff of the change so the side effect is auditable downstream — the + // reviewer stage reads the file_written artifact and needs to see WHAT changed, not just + // that a write happened (F-019). + if (result is ToolResult.Success) { + val updatedContent = runCatching { Files.readString(path) }.getOrDefault("") + val diff = unifiedDiff(pathString, originalContent, updatedContent) + result.copy( + output = if (diff.isBlank()) result.output else "${result.output}\n\n$diff", + metadata = result.metadata + ("diff" to diff), + ) + } else { + result + } } private suspend fun performOperation( diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt index 6a2f2062..02eca108 100644 --- a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt @@ -9,7 +9,6 @@ import com.correx.core.tools.contract.ToolResult import com.correx.core.tools.contract.ValidationResult import kotlinx.coroutines.runBlocking import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test import java.nio.file.Files @@ -95,7 +94,9 @@ class FileReadToolTest { assertTrue(result is ToolResult.Failure) val failure = result as ToolResult.Failure assertTrue(failure.reason.contains("File not found")) - assertFalse(failure.recoverable) + // A missing file is recoverable (F-009): the model often probes for files that may not + // exist; the orchestrator feeds the miss back so it can adapt rather than failing the stage. + assertTrue(failure.recoverable) } @Test diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteToolTest.kt index d74fee66..ebfb8fc7 100644 --- a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteToolTest.kt +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteToolTest.kt @@ -145,7 +145,9 @@ class FileWriteToolTest { assertTrue(result is ToolResult.Success) val success = result as ToolResult.Success - assertEquals("File written successfully to $filePath", success.output) + // Output starts with the status line; a unified diff of the change is appended (F-019). + assertTrue(success.output.startsWith("File written successfully to $filePath")) + assertTrue(success.metadata.containsKey("diff")) assertEquals(invocationId, success.invocationId) assertEquals(content, Files.readString(filePath)) } @@ -167,7 +169,7 @@ class FileWriteToolTest { val result = tool.execute(request) assertTrue(result is ToolResult.Success) val success = result as ToolResult.Success - assertEquals("File deleted successfully: $filePath", success.output) + assertTrue(success.output.startsWith("File deleted successfully: $filePath")) assertEquals(invocationId, success.invocationId) assertFalse(Files.exists(filePath)) } diff --git a/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/TomlWorkflowLoader.kt b/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/TomlWorkflowLoader.kt index afad9464..8e524345 100644 --- a/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/TomlWorkflowLoader.kt +++ b/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/TomlWorkflowLoader.kt @@ -6,6 +6,7 @@ import com.correx.core.artifacts.kind.TypedArtifactSlot import com.correx.core.events.types.ArtifactId import com.correx.core.events.types.StageId import com.correx.core.events.types.TransitionId +import com.correx.core.inference.GenerationConfig import com.correx.core.transitions.graph.StageConfig import com.correx.core.transitions.graph.TransitionEdge import com.correx.core.transitions.graph.WorkflowGraph @@ -91,6 +92,14 @@ class TomlWorkflowLoader( needs = s.needs.map { ArtifactId(it) }.toSet(), allowedTools = s.allowedTools.toSet(), tokenBudget = s.tokenBudget, + // Propagate the declared token budget to the inference completion cap. + // Without this the StageConfig default (maxTokens=2048) is used, truncating + // larger artifacts (finishReason=length) → invalid JSON → validation failure. + generationConfig = GenerationConfig( + temperature = 0.7, + topP = 1.0, + maxTokens = s.tokenBudget, + ), maxRetries = s.maxRetries, metadata = buildMap { s.prompt?.let { put("prompt", resolvePromptPath(it, workflowDir)) } diff --git a/qa/audit-report-2026-06-08.md b/qa/audit-report-2026-06-08.md index e71ac973..ac7b4905 100644 --- a/qa/audit-report-2026-06-08.md +++ b/qa/audit-report-2026-06-08.md @@ -25,6 +25,39 @@ Patches applied this session (rebuild + restart server to load): path resolution. - F-007: validator content-map (drops CAS dep) + `ArtifactContentStoredEvent` slot→hash recording. No server restart needed for validation correctness. +- F-008: `TomlWorkflowLoader` — propagate stage `token_budget` → `generationConfig.maxTokens`. +- F-009: `FileReadTool` file-not-found → `recoverable=true`; `SessionOrchestrator` feeds + recoverable tool errors back into the loop instead of failing the stage. +- F-010: `SessionOrchestrator` — reject premature `stage_complete` when an LLM-emitted slot + is unfilled; nudge the model to emit the artifact. +- F-011: artifact schemas (`analysis`/`design`/`impl_plan`) — list fields `string` → `array`. + Live config under `~/.config/correx/schemas/` + repo `docs/schemas/` synced. +- F-012: `SessionOrchestrator.stripCodeFence` — strip outer ```` ``` ```` fence from + LLM-emitted artifacts before hash/store/validate. +- F-013: `ValidationPipeline` — payload-validation failures retryable, structural ones not. +- F-014 + diff: `FileEditTool` — schema matches real params (`target`/`replacement`/`patch`); + successful edits emit a unified diff. +- F-015: `SessionOrchestrator` — record tool-execution events + materialise a real + `file_written` artifact from the on-disk write; gate validation so a no-write stage cannot + be rubber-stamped. +- F-016: `Main.kt` — `stageTimeoutMs` 60s → 180s (still a hardcode; should be config-wired). +- F-018: `SessionOrchestrator` — write-nudge: when a stage owes a `file_written` artifact but + the model ended a turn with content (or called `stage_complete`) without writing, push back a + corrective tool-result instructing it to invoke `file_edit`/`file_write` (bounded by + `MAX_TOOL_ROUNDS`). Extends the F-010 premature-completion guard to file-write stages. + Regression fix: `FileReadToolTest` updated for the F-009 `recoverable=true` change. +- F-019: `FileWrittenArtifact`/`FileWrittenKind` gain a `diff` field; `file_edit`+`file_write` + emit a unified diff (shared `DiffUtil.unifiedDiff`); `materializeFileWritten` threads it into the + artifact the reviewer reads, so the reviewer can verify the change. +- F-020: `FileEditTool` — model-correctable failures (bad patch, target-not-found, bad params, + unknown op) now `recoverable=true` (feed back, don't abort); tool/`operation` descriptions + + implementer prompt steer toward `replace`/`file_write` over the fragile `patch` op. +- F-021: OPEN (not patched) — resume `rehydrate()` slot-name-vs-CAS-hash bug; see finding. + +**Key reproducibility note:** the live config the server reads is `~/.config/correx/` +(`configDir = ConfigLoader.configPath().parent`), NOT the repo. Code fixes load via repo +build jars on the classpath; all config/prompts/schemas come from `~/.config/correx/`. The +repo `docs/schemas/` were stale relative to live — a clean-checkout reproduction will differ. ## Findings @@ -159,3 +192,280 @@ Patches applied this session (rebuild + restart server to load): `=RetryAttemptedEvent` repeated. Operator sees failure only via router narration, not a structured failure/retry surface in the TUI. - **status:** confirmed +- **note:** `ArtifactContentStoredEvent` is also unmapped (`DomainEventMapper: unmapped + payload type=ArtifactContentStoredEvent`) — same class of gap, new event added this session. + +### F-008 — stage `token_budget` not propagated to provider `max_tokens` → truncation +- **invariant/contract:** functional (workflows run end-to-end) / #7 (validatable output) +- **subsystem:** infrastructure/workflow (TomlWorkflowLoader) + core/transitions (StageConfig) +- **severity:** major +- **evidence:** session `f5046936`: `role_pipeline.toml` analyst declares `token_budget=16384`, + but every request carried `"max_tokens":2048` (the StageConfig default). The analyst's 2nd + inference hit `completionTokens=2048 finishReason=length` → JSON truncated mid-string → + artifact validation failed → `no transition condition matched`. +- **root cause:** `TomlWorkflowLoader.toWorkflowGraph` set `StageConfig.tokenBudget` from the + TOML but never populated `generationConfig`, so the 2048 default `maxTokens` reached the + provider. `token_budget` fed context budgeting only, never the completion cap. +- **status:** FIXED — loader sets `generationConfig.maxTokens = s.tokenBudget`. Confirmed live + (session `1876d22a`: 337 completion tokens, no truncation). + +### F-009 — tool-execution errors abort the stage instead of feeding back to the model +- **invariant/contract:** functional / #3 (tools suggest; core records and continues) +- **subsystem:** infrastructure/tools/filesystem (FileReadTool) + core/kernel (orchestrator loop) +- **severity:** major +- **evidence:** session `1876d22a`: analyst speculatively read `tests/__init__.py` (doesn't + exist); `FileReadTool` returned `recoverable=false` → orchestrator treated it as FATAL → + `stage failed reason=File not found: tests/__init__.py retryable=false`. Both FATAL and + recoverable ERROR tool results aborted the stage (the `ERROR:` feedback entry was built then + discarded) — the model never got to adapt. +- **root cause:** `FileReadTool` marked missing-file `recoverable=false`; `SessionOrchestrator` + loop returned `StageExecutionResult.Failure` for any ERROR/FATAL tool entry instead of + appending recoverable errors to context and continuing. +- **status:** FIXED — missing file → `recoverable=true`; recoverable errors flow back into the + tool loop (bounded by `MAX_TOOL_ROUNDS`); only FATAL aborts. Confirmed live. + +### F-010 — premature `stage_complete` stores an empty artifact → validation fails +- **invariant/contract:** functional / #7 (the artifact-emission contract) +- **subsystem:** core/kernel (SessionOrchestrator) +- **severity:** major (direct consequence of the F-001 grammar drop) +- **evidence:** session `ea968b72`: after reading files the analyst called `stage_complete` + with empty content, never emitting the `analysis` JSON. The orchestrator captured + `response.text` (empty, hash `af1349b9…`) as the artifact → `validation failed`. +- **root cause:** once the GBNF grammar is dropped for tool-bearing stages (F-001), nothing + forces artifact emission; the model can end the stage via `stage_complete` without producing + its declared slot. +- **status:** FIXED — `stage_complete` is rejected while an LLM-emitted slot is unfilled; a + corrective tool-result nudges the model to emit the artifact (bounded by `MAX_TOOL_ROUNDS`). + Confirmed live (session `c02cf17e`: nudge worked, real artifact emitted on the 3rd turn). +- **follow-up (proper fix):** the first-class `emit_artifact` tool from the F-001 TODO would + make artifact production an explicit schema-checked tool call. + +### F-011 — artifact schemas over-constrain list fields as `string` → validation rejects arrays +- **invariant/contract:** functional (artifacts validate) +- **subsystem:** config schemas (`~/.config/correx/schemas/`) + core/validation +- **severity:** major (latent — masked by F-007/F-008/F-010 until artifacts validated cleanly) +- **evidence:** session `c02cf17e`: analyst emitted a complete `analysis` with `requirements` + as a JSON **array**; schema declared `requirements`/`affected_areas` as `"type":"string"` + (newline-separated). `JsonSchemaValidator` (single-type, no unions) rejected the array. + The model emits arrays every run. +- **root cause:** schema design mismatch with natural model output; the minimal validator + cannot express a string|array union. +- **status:** FIXED — `requirements`/`affected_areas` (analysis), `components`/`risks` (design), + `steps`/`verification` (impl_plan) changed `string`→`array`. Live config + repo docs synced. + `review_report.verdict` left `string` (the `verdict == approved` transition depends on it). +- **note:** validation strictness itself is correct (#7 held — it rejected mismatched output). + +### F-012 — LLM wraps artifact JSON in a markdown code fence → unparseable +- **invariant/contract:** functional (artifacts validate) +- **subsystem:** core/kernel (SessionOrchestrator) +- **severity:** major (model-nondeterministic — any stage, any run) +- **evidence:** session `231179c5`: planner emitted ```` ```json … ``` ```` for `impl_plan`; + the validator parses raw JSON → parse failure → `validation failed`. analyst/architect that + run emitted bare JSON and passed — the difference is pure model nondeterminism. +- **status:** FIXED — `stripCodeFence` in `runInference` removes an outer fence wrapper before + the artifact is hashed/stored/validated (CAS, cache, validation stay consistent). Conservative: + only fires when the whole response is a fenced block. + +### F-013 — validation failures never retried → one bad LLM output kills the workflow +- **invariant/contract:** #7 ("Rejected + retried (max_retries)") — the plan's stated contract +- **subsystem:** core/validation (ValidationPipeline) +- **severity:** BLOCKER (robustness — the dominant failure mode with a nondeterministic model) +- **evidence:** session `67776482`: analyst emitted a malformed artifact (a tool-call shape as + JSON content); validation rejected it `retryable=false` and the workflow died, despite + `max_retries=2`. The previous run's analyst was flawless — pure nondeterminism with zero + recovery. +- **root cause:** `ValidationPipeline` marked **all** ERROR-severity validation outcomes + `retryable=false` ("structural errors must be fixed"), conflating graph misconfiguration with + transient bad LLM output. +- **status:** FIXED — payload-validation failures (`artifact_payload` section) are now retryable + (stage re-runs up to `max_retries`); structural/graph errors stay terminal. This wired the + invariant-#7 contract that was never actually implemented. Stabilised the pipeline end-to-end + (session `c6212fd0` completed 9 stages after this). + +### F-014 — `file_edit` schema misdescribes its own parameters → edits never apply +- **invariant/contract:** functional (tool side effects) / #5 +- **subsystem:** infrastructure/tools/filesystem (FileEditTool) +- **severity:** BLOCKER +- **evidence:** session `c6212fd0`: implementer called `file_edit operation=patch` with + `old_str`/`new_str` (exactly what the schema advertised) → `Missing 'patch' parameter for + 'patch' operation`. Across all 4 implementer rounds, not one edit applied; the file stayed + unchanged. +- **root cause:** schema documented `content`/`old_str`/`new_str`; the code reads `content` + (append), `target`/`replacement` (replace), `patch` (unified diff, via `patch -p1`). + `old_str`/`new_str` are read by nothing; `target`/`replacement`/`patch` are documented + nowhere. A schema-following model can never `replace` or `patch`. +- **status:** FIXED — schema now documents the real params (`target`/`replacement`/`patch`), + drops the dead `old_str`/`new_str`. Plus: successful edits emit a `diff -u` unified diff + (operator-requested), surfaced in `ToolReceipt.diff` and the `file_written` artifact. + +### F-015 — tool-output artifacts auto-validated without verifying the side effect → false success +- **invariant/contract:** #5 (all side effects captured in events) / functional (real output) +- **subsystem:** core/kernel (SessionOrchestrator emitToolArtifacts + dispatch) +- **severity:** BLOCKER (a green `done` run that changed nothing — worst-case for a kernel) +- **evidence:** session `c6212fd0`: workflow completed `terminalStage=done, verdict=approved, + 9 stages`, but `git status` clean and `calc.average` bug intact. `emitToolArtifacts` + unconditionally emitted `ArtifactCreated→Validating→Validated` for the `patch` slot every + implementer round regardless of any write; `validateSlotAsFileWritten` early-returns when the + slot has no content (it never had any) → rubber-stamped. The reviewer then approved a no-op. +- **root cause:** file-mutating tools never produced a `file_written` artifact (unlike shell + tools → `ProcessResultArtifact`); tool executions emitted no completion/file-write events at + all, so nothing tied the `patch` slot to a real write. +- **status:** FIXED — dispatch now emits `ToolExecutionCompleted`/`Failed` + `FileWrittenEvent`, + and **materialises a real `file_written` artifact** (path, content hash, size, diff) from the + on-disk result. `emitToolArtifacts` gates the `file_written` slot on that content: no write → + no artifact → `verifyProduces` fails the stage → retry → honest failure (confirmed live, + session `57d5795d`: `did not produce declared artifacts: patch retryExhausted=true`). + +### F-016 — `stageTimeoutMs` hardcoded at 60s, too tight for local models + not configurable +- **invariant/contract:** functional (operability) +- **subsystem:** core/kernel (OrchestrationConfig) + apps/server (Main) +- **severity:** major +- **evidence:** session `57d5795d`: ~15 `TIMEOUT: Timed out waiting for 60000 ms` across the + run (7–8 on implementer alone). Large implementer/reviewer turns (plan + design + file + contents) were cut off mid-generation, burning all retries → `did not produce patch`. +- **root cause:** `OrchestrationConfig.stageTimeoutMs` defaults to `60_000L` and is hardcoded + in `Main` (`defaultOrchestrationConfig`) with no config wiring. +- **status:** PARTIAL — raised to `180_000L` in `Main`. Should be surfaced as a config knob + (TODO left at the patch site). + +### F-017 (observation, not a correx defect) — local model does not reliably issue write tools +- **invariant/contract:** n/a (model capability) +- **subsystem:** n/a (provider model `llama-cpp:default`) +- **severity:** informational +- **evidence:** session `57d5795d` implementer: 6 inference attempts, **only `file_read` calls** + — zero `file_edit`/`file_write`, zero `stage_complete`. Even on turns that completed within + timeout, the model read repeatedly and never attempted the edit it was instructed to make. +- **note:** correx offers `file_write`/`file_edit` correctly and the implementer prompt + explicitly instructs tool-based writing. This is a model-selection conclusion: role_pipeline's + correctness is sound, but this local model is too weak for the implementer stage. Re-test the + implementer with a stronger model before drawing functional conclusions about that stage. + +--- + +### F-018 — implementer emits the change as prose instead of invoking the write tool +- **invariant/contract:** functional (workflows write expected files) — mitigation for F-017 +- **subsystem:** core/kernel (SessionOrchestrator tool loop) +- **severity:** major (robustness; the stage silently failed instead of coaxing a write) +- **evidence:** sessions `57d5795d`, `fef41879`: implementer dispatched only `file_read` + (2–3 calls), then ended turns with `finishReason=stop`/`length` — emitting the corrected code + as message content, never calling `file_edit`/`file_write`. The write tools WERE offered in the + request (confirmed: `file_edit`/`file_write` present in the implementer's tools array). The + stage then failed `did not produce declared artifacts: patch` with no attempt to redirect the + model. +- **status:** FIXED (staged) — the tool loop now nudges: a content turn (or a `stage_complete`) + while a `file_written` slot is unfilled triggers a corrective tool-result ("you MUST call + file_edit/file_write …") and a re-run, bounded by `MAX_TOOL_ROUNDS`. If the model never + complies it fails honestly at the round cap — a clean signal that the model (not correx) is the + wall. Unit tests green; awaiting a live run to confirm it tips this model into tool-calling. + +### F-019 — `file_written` artifact carries only metadata → reviewer can't see the change, loops forever +- **invariant/contract:** functional (refinement loop / reviewer can verify work) +- **subsystem:** core/artifacts (FileWrittenArtifact) + core/kernel (needs-injection) + the diff path +- **severity:** major +- **evidence:** session `265f5fe6` — the implementer **wrote correctly** (`file_write`, real diff; + `calc.average` fixed, all 7 pytest pass). But the reviewer (`needs=[patch, impl_plan]`) saw only + the `FileWrittenArtifact` metadata and returned `changes_requested`: *"Cannot verify the patch + content — only metadata (hash, size, mode) was provided, not the actual code changes."* The + refinement loop hit its 2-iteration cap → `WorkflowFailed: refinement loop exceeded 2 iterations`. +- **root cause:** `FileWrittenArtifact` is `(path, contentHash, size, mode)` — no diff, no content. + The diff added in F-014 lives on `ToolReceipt.diff` / the tool output, but is NOT threaded into + the artifact the reviewer's `needs` injection reads. (Also: the implementer used `file_write`, + which produces no diff at all.) So the reviewer is structurally unable to review the change — the + flip side of the operator's original "patch tool should provide a diff" point. +- **status:** FIXED (staged) — `FileWrittenArtifact` gained a `diff` field; both `file_edit` and + `file_write` now compute a `diff -u` unified diff (shared `unifiedDiff` helper in + `DiffUtil.kt`) and surface it via `ToolResult.metadata["diff"]`; `materializeFileWritten` + threads it into the artifact the reviewer's `needs`-injection reads. Unit tests green (artifacts + 6, filesystem 38, kernel, validation). Awaiting a live run to confirm the reviewer can now verify + the change and the loop converges. +- **secondary observation:** reviewer verdicts were nondeterministic — one turn emitted + `verdict=approved` ("approved at tier T3…"), later turns `changes_requested`. With no diff to + anchor on, the verdict is essentially ungrounded. + +### F-020 — `file_edit` failures are fatal + model defaults to the fragile `patch` op +- **invariant/contract:** functional (tool robustness) / #3 (tools suggest, core records & continues) +- **subsystem:** infrastructure/tools/filesystem (FileEditTool) + implementer prompt +- **severity:** major +- **evidence:** session `6ebe0c50` — the implementer correctly called `file_edit` (F-014 works) + but chose `operation=patch` and produced a non-applying unified diff → `patch -p1` exit 2 → + `Patch failed with exit code 2`. The failure was `recoverable=false` (FATAL) → the stage aborted + with no chance for the model to fall back to `replace`/`file_write`. File unchanged. + (Also observed: a client-side `Request timeout has expired` independent of `stageTimeoutMs`.) +- **root cause:** (a) model-correctable `file_edit` failures (bad patch, target-not-found, bad + params, unknown op) were all marked `recoverable=false`, so they aborted instead of feeding the + error back (same class as F-009); (b) nothing steered the finicky local model away from the + fragile `patch` op toward the reliable `replace`/`file_write`. +- **status:** FIXED (staged) — (a) those failures are now `recoverable=true` → fed back into the + loop so the model can self-correct or fall back (and F-018's write-nudge re-engages); the patch + failure message now explicitly suggests `replace`/file_write. (b) Tool description + `operation` + description recommend `replace`/file_write over `patch`; implementer prompt rewritten to call a + write tool (prefer file_write/replace, avoid patch) and not `stage_complete` before writing. + Filesystem tests green (38). Awaiting a live run. +- **note (model-swap groundwork):** correx already has per-stage model selection in the data model + (`StageConfig.modelId`, capability routing, multi-provider config) but it is **unwired** — the + TOML loader parses neither a per-stage `model` nor `capabilities`, and `DefaultInferenceRouter`'s + `modelId` overload falls back to capability routing. Enabling the llm_pipeline-style per-stage + model swap (e.g. a tool-strong model for implementer/reviewer) needs only targeted wiring, not + new architecture. Deferred pending the robustness-first decision. + +### F-021 — resume-after-restart `rehydrate()` reuses the F-007 slot-name-vs-CAS-hash conflation +- **invariant/contract:** #1 (event log is the only source of truth) / #8 (replay/restore is + environment-independent) — resume-after-restart +- **subsystem:** core/kernel (DefaultSessionOrchestrator.rehydrate) +- **severity:** major +- **evidence (static, not yet probed live):** `DefaultSessionOrchestrator.rehydrate` (line 171) + walks `ArtifactValidatedEvent`s and calls `artifactStore.get(p.artifactId)` to repopulate + `artifactContentCache`. `p.artifactId` is the **logical slot id** ("analysis", "patch"), but + `CasArtifactStore.get` requires a content **hash** (`id.value.hexToBytes()`). `get("analysis")` + → non-hex → throws → `rehydrate` throws → `resumeSession (rehydrate)` logs failure. So a + mid-session resume cannot restore produced-artifact content; downstream `needs`-injection and + artifact validation (which read `artifactContentCache`) would then be empty/broken. +- **root cause:** identical to F-007 — slot name vs CAS content-hash. The F-007 fix added + `ArtifactContentStoredEvent(artifactId=slot, contentHash=hash)` as the durable bridge, but + `rehydrate` does not consult it; it still passes the slot id straight to the hash-keyed CAS. +- **fix direction:** rehydrate from `ArtifactContentStoredEvent` — `artifactStore.get(contentHash)` + then `artifactContentCache["sessionId:slot"] = content`. The F-007/F-015 events already record + the needed slot→hash mapping; this just consumes it on cold start. +- **status:** OPEN (found by code inspection while answering "is resume actually implemented?"). + Replay (`ReplayOrchestrator` + `testing/replay/` suite) is implemented and unit-tested; resume's + wiring exists (`POST /sessions/{id}/resume`, `resume()`, `rehydrate()`) but this bug means it + almost certainly does not restore content correctly — needs a live #1/#8 probe to confirm. + +### Steering (functional contract) — partially exercised live +- During the live operator sessions, approval-with-steering was informally validated: the operator + steered toward a specific indentation; the initial attempt was made without that guidance and the + steered re-attempt incorporated it. So the steering note entered the journal and affected the + run. Not yet a controlled probe (no event-level assertion captured), but not untouched either. + +## Audit outcome summary + +role_pipeline is now **fully green, end-to-end**. Session `4679ed1e` (the final run) completed +`WorkflowCompleted, terminalStage=done, 5 stages` in a single clean pass — analyst → architect → +planner → implementer → reviewer → done, **no review-loop bounce**: +- the implementer followed the F-020 steering and used `file_write` (4 calls, 3 `FileWrittenEvent`s, + no `patch` botching); +- `calc.average` was correctly fixed (`if len(values) == 0: return 0.0`), real git diff, **all 7 + pytest pass**; +- the `file_written` artifact carried the **diff** (F-019), the reviewer's `needs`-injection showed + `Input artifact: patch` *with the diff*, so the reviewer could actually verify the change and + **approved on the first pass**. + +Every correctness defect found (F-001, F-005, F-007 through F-016, F-018, F-019, F-020) is fixed +and now confirmed live. The earlier failures (`265f5fe6` reviewer loop, `6ebe0c50` bad patch) were +the last two blockers; both are closed. + +**Still to PROBE** (implementations mostly exist; the audit hasn't run the adversarial checks — +"outstanding" here means un-probed, not unimplemented): +- **#8 offline replay** — *implemented and unit-tested* (`ReplayOrchestrator` + `testing/replay/` + suite). Needs the adversarial probe (export log, replay with network/LLM off) to confirm. +- **#1 resume-after-restart** — *wiring implemented* (`POST /sessions/{id}/resume`, `resume()`, + `rehydrate()`) but **F-021 shows `rehydrate()` is broken** (slot-name-vs-CAS-hash, same as + F-007). Strongly expected to fail a live probe until F-021 is fixed. +- **#2** projection isolation, **#4** policy-vs-approval, **#6** compression non-authority, + **#9** env-observation replay — not yet probed this audit. +- **undo / cancel / approval-gate** — functional contracts not yet exercised. **Steering** was + *partially* exercised live (operator steered indentation; re-attempt incorporated it). + +There is now a real recorded, validated file change (`4679ed1e`) to point the undo/replay/resume +probes at — though resume needs F-021 first. **Optional groundwork:** wire per-stage model +selection (F-020 note); the model remained the flaky element even on this passing run.