fix(kernel,tools,validation): unblock role_pipeline end-to-end + QA audit
Live QA audit of role_pipeline against a sample repo surfaced and fixed a chain of defects that prevented any tool+artifact workflow from running end-to-end. The pipeline now completes cleanly with a real, validated, reviewed file change. - F-008 workflow: propagate stage token_budget -> generationConfig.maxTokens (TomlWorkflowLoader) so large stages stop truncating at the 2048 default. - F-009 tools/kernel: file_read missing-file is recoverable; recoverable tool errors feed back into the loop instead of aborting the stage. - F-010/F-018 kernel: reject premature stage_complete and nudge the model to emit the owed artifact / invoke a write tool (bounded by MAX_TOOL_ROUNDS). - F-011 schemas: list-typed artifact fields string -> array (analysis/design/ impl_plan) to match model output. - F-012 kernel: strip an outer markdown code fence from LLM artifacts. - F-013 validation: payload-validation failures are retryable; structural failures stay terminal (wires the invariant-#7 reject+retry contract). - F-014 tools: file_edit schema matches its real params; edits emit a diff. - F-015 kernel: 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 (no more false-success runs). - F-016 server: raise stageTimeoutMs 60s -> 180s for slow local models. - F-019 artifacts/kernel: file_written artifact carries the unified diff so the reviewer can verify the change (shared DiffUtil; file_write emits a diff too). - F-020 tools: model-correctable file_edit failures are recoverable + steer toward replace/file_write over the fragile patch op. Full findings register (F-001..F-021, incl. open F-021 resume rehydrate bug) in qa/audit-report-2026-06-08.md. All module tests green.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
+214
-12
@@ -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<ContextEntry> {
|
||||
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<TypedArtifactSlot>,
|
||||
) {
|
||||
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<java.nio.file.Path>,
|
||||
fileWrittenSlots: List<TypedArtifactSlot>,
|
||||
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,
|
||||
|
||||
+8
-2
@@ -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<Validator>,
|
||||
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)
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -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"],
|
||||
|
||||
@@ -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"],
|
||||
|
||||
+36
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
+41
-13
@@ -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 {
|
||||
|
||||
+3
-1
@@ -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 {
|
||||
|
||||
+16
-1
@@ -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(
|
||||
|
||||
+3
-2
@@ -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
|
||||
|
||||
+4
-2
@@ -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))
|
||||
}
|
||||
|
||||
+9
@@ -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)) }
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user