From f51a8dada42a3ff723abee740d79d444ad358760 Mon Sep 17 00:00:00 2001 From: kami Date: Wed, 8 Jul 2026 10:28:53 +0400 Subject: [PATCH] feat(recovery): route failed write-less stages to recovery instead of futile retry Adds the failure-ticket + recovery-routing mechanism: a deterministic gate->capability table gates whether a stage has agency to fix its own failure, opens a FailureTicketOpenedEvent when it doesn't, and routes to a metadata role=recovery stage (per-stage budget, cap 2, not reset by TransitionExecuted) instead of retrying in place. Extends salvage decisions with a RECOVER option so the review-gate judge can also route to recovery, unifies deterministic-gate and review-gate routing through routeToRecovery(), and has ExecutionPlanCompiler synthesize a write-capable recovery stage + edge for freestyle plans. Read-only tools (file_read, list_dir) are now always available on any tool-granting stage so a recovery stage can inspect the write-less stage's failure without flooding context via shell ls -R. --- .../apps/server/bridge/DomainEventMapper.kt | 23 +++++++++ .../com/correx/apps/server/protocol/Dtos.kt | 11 +++++ .../apps/server/protocol/ServerMessage.kt | 13 +++++ .../server/bridge/DomainEventMapperTest.kt | 5 +- apps/tui-go/internal/app/model.go | 23 +++++++++ .../tui-go/internal/app/render_matrix_test.go | 15 ++++++ apps/tui-go/internal/app/server.go | 47 ++++++++++++++++++- apps/tui-go/internal/app/update.go | 12 ++++- apps/tui-go/internal/app/view.go | 5 +- apps/tui-go/internal/protocol/protocol.go | 23 ++++++++- .../core/artifacts/kind/KindContractTable.kt | 9 +++- .../artifacts/kind/KindContractTableTest.kt | 26 ++++++++++ .../core/events/events/OrchestrationEvents.kt | 10 +++- .../orchestration/OrchestrationState.kt | 7 +++ .../DefaultOrchestrationReducer.kt | 6 ++- .../DefaultSessionOrchestrator.kt | 23 +++++++-- .../orchestration/SessionOrchestrator.kt | 7 ++- .../tools/filesystem/ListDirTool.kt | 18 ++++--- .../tools/filesystem/ListDirToolTest.kt | 15 +++++- .../workflow/ExecutionPlanCompiler.kt | 13 +++++ .../src/test/kotlin/RecoveryRoutingTest.kt | 43 +++++++++++++++-- 21 files changed, 325 insertions(+), 29 deletions(-) diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/bridge/DomainEventMapper.kt b/apps/server/src/main/kotlin/com/correx/apps/server/bridge/DomainEventMapper.kt index 5caab713..14447809 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/bridge/DomainEventMapper.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/bridge/DomainEventMapper.kt @@ -2,6 +2,7 @@ package com.correx.apps.server.bridge import com.correx.apps.server.protocol.AssessedIssueDto import com.correx.apps.server.protocol.PauseReason +import com.correx.apps.server.protocol.ReviewFindingDto import com.correx.apps.server.protocol.RiskSummaryDto import com.correx.apps.server.protocol.ServerMessage import com.correx.apps.server.protocol.toDto @@ -22,6 +23,7 @@ import com.correx.core.events.events.WorkflowStartedEvent import com.correx.core.events.events.InferenceFailedEvent import com.correx.core.events.events.InferenceStartedEvent import com.correx.core.events.events.InferenceTimeoutEvent +import com.correx.core.events.events.ReviewFindingsRaisedEvent import com.correx.core.events.events.RetryAttemptedEvent import com.correx.core.events.events.ModelLoadedEvent import com.correx.core.events.events.ModelUnloadedEvent @@ -190,6 +192,7 @@ suspend fun domainEventToServerMessage( outputSummary = p.receipt.outputSummary, occurredAt = event.metadata.timestamp.toEpochMilliseconds(), diff = p.receipt.diff, + affectedEntities = p.receipt.affectedEntities, sequence = seq, sessionSequence = sessionSequence, ) @@ -222,6 +225,26 @@ suspend fun domainEventToServerMessage( sessionSequence = sessionSequence, ) + is ReviewFindingsRaisedEvent -> ServerMessage.ReviewFindings( + sessionId = p.sessionId, + stageId = p.stageId, + verdict = p.verdict.name, + findings = p.findings.map { + ReviewFindingDto( + severity = it.severity.name, + confidence = it.confidence, + category = it.category, + target = it.target, + message = it.message, + suggestedFix = it.suggestedFix, + correctness = it.correctness, + ) + }, + blocked = p.blocked, + sequence = seq, + sessionSequence = sessionSequence, + ) + is ApprovalRequestedEvent -> mapApprovalRequested(p, seq, sessionSequence) is ClarificationRequestedEvent -> ServerMessage.ClarificationRequired( sessionId = p.sessionId, diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/protocol/Dtos.kt b/apps/server/src/main/kotlin/com/correx/apps/server/protocol/Dtos.kt index e0b7aae2..3e26a697 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/protocol/Dtos.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/protocol/Dtos.kt @@ -19,6 +19,17 @@ data class AssessedIssueDto( val severity: String, ) +@Serializable +data class ReviewFindingDto( + val severity: String, + val confidence: Double, + val category: String, + val target: String, + val message: String, + val suggestedFix: String? = null, + val correctness: Boolean = false, +) + @Serializable data class ProviderHealthDto( val providerId: String, diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/protocol/ServerMessage.kt b/apps/server/src/main/kotlin/com/correx/apps/server/protocol/ServerMessage.kt index da415f48..94651bcd 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/protocol/ServerMessage.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/protocol/ServerMessage.kt @@ -246,6 +246,7 @@ sealed interface ServerMessage { val outputSummary: String, val occurredAt: Long, val diff: String? = null, + val affectedEntities: List = emptyList(), override val sequence: Long, override val sessionSequence: Long, ) : ServerMessage, SessionMessage @@ -284,6 +285,18 @@ sealed interface ServerMessage { override val sessionSequence: Long, ) : ServerMessage, SessionMessage + @Serializable + @SerialName("review.findings") + data class ReviewFindings( + val sessionId: SessionId, + val stageId: StageId, + val verdict: String, + val findings: List, + val blocked: Boolean, + override val sequence: Long, + override val sessionSequence: Long, + ) : ServerMessage, SessionMessage + // -- Approval -- @Serializable diff --git a/apps/server/src/test/kotlin/com/correx/apps/server/bridge/DomainEventMapperTest.kt b/apps/server/src/test/kotlin/com/correx/apps/server/bridge/DomainEventMapperTest.kt index b622896f..5c9e43af 100644 --- a/apps/server/src/test/kotlin/com/correx/apps/server/bridge/DomainEventMapperTest.kt +++ b/apps/server/src/test/kotlin/com/correx/apps/server/bridge/DomainEventMapperTest.kt @@ -349,7 +349,10 @@ class DomainEventMapperTest { ) val result = domainEventToServerMessage(event, noopStore, sessionSequence = 0L) assertEquals( - ServerMessage.ToolCompleted(sessionId, "file_write", "wrote 3 lines", occurredAt, diff = null, event.sequence, 0L), + ServerMessage.ToolCompleted( + sessionId, "file_write", "wrote 3 lines", occurredAt, + diff = null, affectedEntities = emptyList(), event.sequence, 0L, + ), result, ) } diff --git a/apps/tui-go/internal/app/model.go b/apps/tui-go/internal/app/model.go index 8f4c4b44..96a92974 100644 --- a/apps/tui-go/internal/app/model.go +++ b/apps/tui-go/internal/app/model.go @@ -211,6 +211,29 @@ type Session struct { Clar *Clarification // open questions awaiting answers (clarification view) Propose *Proposal // router workflow suggestion awaiting a pick (propose view) Active bool // an inference/tool is in flight (drives the spinner) + + // LastReview is the most recent semantic reviewer (Gate 3) verdict for this + // session, from review.findings. Nil until the first review lands. + LastReview *ReviewResult +} + +// ReviewResult is the semantic reviewer's verdict + findings over a stage's +// produced files (mirrors ReviewFindingsRaisedEvent). +type ReviewResult struct { + StageID string + Verdict string // PASS | WARN | FAIL + Findings []ReviewFinding + Blocked bool +} + +type ReviewFinding struct { + Severity string + Confidence float64 + Category string + Target string + Message string + SuggestedFix string + Correctness bool } // Workflow is a launchable workflow advertised by the server. diff --git a/apps/tui-go/internal/app/render_matrix_test.go b/apps/tui-go/internal/app/render_matrix_test.go index ac2d5251..f5e2edab 100644 --- a/apps/tui-go/internal/app/render_matrix_test.go +++ b/apps/tui-go/internal/app/render_matrix_test.go @@ -296,6 +296,21 @@ func matrixCases() []matrixCase { }, want: []string{"ToolAssessed", "BLOCK", "PATH_ESCAPE"}, }, + { + name: "review.findings", + kinds: []string{protocol.TypeReviewFindings}, + surface: surfaceEvents, + build: func() protocol.ServerMessage { + return msg(protocol.TypeReviewFindings, withStage("write_script"), func(m *protocol.ServerMessage) { + m.ReviewVerdict = "FAIL" + m.ReviewBlocked = true + m.ReviewFindings = []protocol.ReviewFindingDto{ + {Severity: "CRITICAL", Confidence: 0.9, Category: "correctness", Target: "main.go:12", Message: "nil deref"}, + } + }) + }, + want: []string{"ReviewFindings", "write_script", "FAIL"}, + }, // --- artifacts / plan (event stream rows) --- { diff --git a/apps/tui-go/internal/app/server.go b/apps/tui-go/internal/app/server.go index c7d14c3b..ce0f4e67 100644 --- a/apps/tui-go/internal/app/server.go +++ b/apps/tui-go/internal/app/server.go @@ -221,7 +221,52 @@ func (m *Model) applyServer(msg protocol.ServerMessage) { m.appendAction(msg.SessionID, "✎", "wrote "+path+countSuffix(add, del)) m.appendRouter(msg.SessionID, RouterEntry{Role: "tool", Content: *msg.Diff}) } else { - m.appendAction(msg.SessionID, "✓", actionToolText(msg.ToolName, msg.Summary)) + label := msg.ToolName + if len(msg.AffectedEntities) > 0 { + label += " " + strings.Join(msg.AffectedEntities, ", ") + } + m.appendAction(msg.SessionID, "✓", actionToolText(label, msg.Summary)) + } + case protocol.TypeReviewFindings: + if s := m.session(msg.SessionID); s != nil { + findings := make([]ReviewFinding, 0, len(msg.ReviewFindings)) + for _, f := range msg.ReviewFindings { + fix := "" + if f.SuggestedFix != nil { + fix = *f.SuggestedFix + } + findings = append(findings, ReviewFinding{ + Severity: f.Severity, + Confidence: f.Confidence, + Category: f.Category, + Target: f.Target, + Message: f.Message, + SuggestedFix: fix, + Correctness: f.Correctness, + }) + } + s.LastReview = &ReviewResult{ + StageID: msg.StageID, + Verdict: msg.ReviewVerdict, + Findings: findings, + Blocked: msg.ReviewBlocked, + } + detail := msg.ReviewVerdict + if len(findings) > 0 { + detail += " (" + plural(len(findings), "finding") + ")" + } + if msg.ReviewBlocked { + detail += " · blocked" + } + s.LastOutput = "review: " + detail + s.addEvent(nowMillis(), "ReviewFindings", msg.StageID+": "+detail) + icon := "✓" + if msg.ReviewVerdict == "FAIL" { + icon = "✕" + } else if msg.ReviewVerdict == "WARN" { + icon = "▲" + } + m.appendAction(msg.SessionID, icon, "review "+detail) } case protocol.TypeToolAssessed: if s := m.session(msg.SessionID); s != nil { diff --git a/apps/tui-go/internal/app/update.go b/apps/tui-go/internal/app/update.go index ca62fdec..550e53fe 100644 --- a/apps/tui-go/internal/app/update.go +++ b/apps/tui-go/internal/app/update.go @@ -225,7 +225,11 @@ func (m Model) handleKey(k keyMsg) (tea.Model, tea.Cmd) { // On the idle launcher, Tab cycles the launch target (chat → workflows → chat) in any // edit mode, so it works whether or not the input is focused. if m.displayState() == StateIdle && k.Type == keyTab { - m.cycleLauncherWf() + if k.Shift { + m.cycleLauncherWfBack() + } else { + m.cycleLauncherWf() + } return m, nil } if m.editMode == ModeInsert { @@ -1290,6 +1294,12 @@ func (m *Model) cycleLauncherWf() { m.launcherWf = (m.launcherWf + 1) % (len(m.workflows) + 1) } +// cycleLauncherWfBack is cycleLauncherWf in reverse, for Shift+Tab. +func (m *Model) cycleLauncherWfBack() { + n := len(m.workflows) + 1 + m.launcherWf = (m.launcherWf - 1 + n) % n +} + // cycleRightPanel advances the in-session right panel: events → changes → off → events. func (m *Model) cycleRightPanel() { m.rightPanel = (m.rightPanel + 1) % 3 diff --git a/apps/tui-go/internal/app/view.go b/apps/tui-go/internal/app/view.go index e0e0eb4f..e31f0850 100644 --- a/apps/tui-go/internal/app/view.go +++ b/apps/tui-go/internal/app/view.go @@ -908,7 +908,7 @@ func shortPath(p string) string { // fill left-justifies a styled line and pads with bg to width w. func (m Model) fill(s string, w int, bg color.Color) string { - return " " + padTo(s, w-1, bg) + return lipgloss.NewStyle().Background(bg).Render(" ") + padTo(s, w-1, bg) } // justify places left and right segments on a bg-filled line of width w, truncating @@ -936,7 +936,8 @@ func (m Model) justify(left, right string, w int, bg color.Color) string { mid = 1 } pad := lipgloss.NewStyle().Background(bg).Render(strings.Repeat(" ", mid)) - return " " + left + pad + right + lipgloss.NewStyle().Background(bg).Render(" ") + edge := lipgloss.NewStyle().Background(bg).Render(" ") + return edge + left + pad + right + edge } // padTo pads (or truncates) a possibly-styled string to visible width w, filling diff --git a/apps/tui-go/internal/protocol/protocol.go b/apps/tui-go/internal/protocol/protocol.go index 054d2ed3..8e1b8794 100644 --- a/apps/tui-go/internal/protocol/protocol.go +++ b/apps/tui-go/internal/protocol/protocol.go @@ -60,6 +60,7 @@ const ( TypeHealthChecks = "health.checks" TypeFileList = "file.list" TypeGrantList = "grant.list" + TypeReviewFindings = "review.findings" ) // ServerMessage is a flat decode of every server->client variant. Field names @@ -82,7 +83,9 @@ type ServerMessage struct { LastResponse string `json:"lastResponse"` // session_snapshot: last stage response (reopen) Content string `json:"content"` Diff *string `json:"diff"` - Tier string `json:"tier"` + // tool.completed: file/dir path(s) the tool read, wrote, or listed. + AffectedEntities []string `json:"affectedEntities"` + Tier string `json:"tier"` Message string `json:"message"` RequestID string `json:"requestId"` ArtifactID string `json:"artifactId"` @@ -136,6 +139,11 @@ type ServerMessage struct { AssessedIssues []AssessedIssueDto `json:"issues"` Artifacts []ArtifactDto `json:"artifacts"` + // review.findings + ReviewVerdict string `json:"verdict"` + ReviewFindings []ReviewFindingDto `json:"findings"` + ReviewBlocked bool `json:"blocked"` + // config.snapshot ConfigFields []ConfigFieldDto `json:"fields"` ConfigRestartRequired []string `json:"restartRequired"` @@ -331,6 +339,17 @@ type AssessedIssueDto struct { Severity string `json:"severity"` } +// ReviewFindingDto is a PR-comment-style finding from the semantic reviewer (Gate 3). +type ReviewFindingDto struct { + Severity string `json:"severity"` + Confidence float64 `json:"confidence"` + Category string `json:"category"` + Target string `json:"target"` + Message string `json:"message"` + SuggestedFix *string `json:"suggestedFix"` + Correctness bool `json:"correctness"` +} + type ProviderHealthDto struct { ProviderID string `json:"providerId"` Status string `json:"status"` @@ -396,7 +415,7 @@ func (m ServerMessage) IsEventBearing() bool { TypeWorkflowProposed, TypeWorkspaceBound, TypeArtifactCreated, TypeArtifactValid, TypePlanLocked, - TypeRouterNarration: + TypeRouterNarration, TypeReviewFindings: return true default: return false diff --git a/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/KindContractTable.kt b/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/KindContractTable.kt index fc2600e2..baf5afe3 100644 --- a/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/KindContractTable.kt +++ b/core/artifacts/src/main/kotlin/com/correx/core/artifacts/kind/KindContractTable.kt @@ -145,6 +145,13 @@ object KindContractTable { val overrides = projectOverrides[kindId].orEmpty() .filter { ov -> base.none { it.id == ov.id } } .map { it.copy(target = path) } - return base + overrides + // A dotfile (basename begins with '.') may legitimately be empty: .gitkeep/.keep directory + // placeholders, .nojekyll markers, an empty .gitignore. Asserting file_nonempty on one makes + // the contract unsatisfiable by construction — a real .gitkeep IS empty — which live-locks the + // producing stage rewriting the same placeholder every retry (observed: scaffold_frontend + // looping on four src/*/.gitkeep files). file_exists still applies, so the file must exist. + return (base + overrides).filterNot { it.id == "file_nonempty" && isDotfile(path) } } + + private fun isDotfile(path: String): Boolean = path.substringAfterLast('/').startsWith(".") } diff --git a/core/artifacts/src/test/kotlin/com/correx/core/artifacts/kind/KindContractTableTest.kt b/core/artifacts/src/test/kotlin/com/correx/core/artifacts/kind/KindContractTableTest.kt index 052acd2f..7bf51435 100644 --- a/core/artifacts/src/test/kotlin/com/correx/core/artifacts/kind/KindContractTableTest.kt +++ b/core/artifacts/src/test/kotlin/com/correx/core/artifacts/kind/KindContractTableTest.kt @@ -61,6 +61,32 @@ class KindContractTableTest { assertTrue("registered_in_serialization" in ids) } + @Test + fun `dotfiles are exempt from file_nonempty but still must exist`() { + // Regression: a .gitkeep placeholder is empty by definition, so a file_nonempty floor made + // its contract unsatisfiable and live-locked scaffold_frontend rewriting src/*/.gitkeep every + // retry. Any dotfile (basename begins with '.') may legitimately be empty. + val gitkeep = KindContractTable.assertionsFor( + KindInference.kindFor("frontend/src/hooks/.gitkeep") ?: "", + "frontend/src/hooks/.gitkeep", + ).map { it.id } + assertTrue("file_exists" in gitkeep, "dotfile must still be required to exist") + assertTrue("file_nonempty" !in gitkeep, "an empty .gitkeep must not fail file_nonempty") + + // A non-dotfile with the same unknown kind keeps the full floor. + val regular = KindContractTable.assertionsFor("", "frontend/src/hooks/placeholder.ts").map { it.id } + assertTrue("file_nonempty" in regular, "non-dotfiles keep the nonempty floor") + } + + @Test + fun `dotfile exemption also strips a would-be file_nonempty project override`() { + KindContractTable.projectOverrides = mapOf( + "" to listOf(ContractAssertion("file_nonempty", "", AssertionEvaluator.FS)), + ) + val ids = KindContractTable.assertionsFor("", "config/.nojekyll").map { it.id } + assertTrue("file_nonempty" !in ids, "dotfile exemption is final, even against overrides") + } + @Test fun `project override is additive and cannot remove checked-in assertions`() { KindContractTable.projectOverrides = mapOf( diff --git a/core/events/src/main/kotlin/com/correx/core/events/events/OrchestrationEvents.kt b/core/events/src/main/kotlin/com/correx/core/events/events/OrchestrationEvents.kt index 221c4e99..99c1c7d7 100644 --- a/core/events/src/main/kotlin/com/correx/core/events/events/OrchestrationEvents.kt +++ b/core/events/src/main/kotlin/com/correx/core/events/events/OrchestrationEvents.kt @@ -105,8 +105,16 @@ data class FailureTicketOpenedEvent( val routeTo: StageId, // The gate's captured failure output (command/exit/stderr/findings) that recovery must resolve. val evidence: String, - // 1-based count of times this stage has been routed to recovery (own, small route budget). + // Count of NO-PROGRESS routes charged against this stage's recovery budget so far. A route whose + // failure fingerprint changed from the previous route (recovery made progress — a different error + // now) is FREE and leaves this flat; only a route that reproduces the same failure charges it. So + // a multi-cause failure repaired one layer at a time is not penalised for progressing. Folded + // directly by the reducer (idempotent under replay). Defaulted for backward-compatible replay of + // pre-progress-aware tickets. val routeAttempt: Int, + // Fingerprint of [evidence] (whitespace/digit-normalised, see FailureFingerprint) at route time. + // The next route compares against this to decide progress vs. no-progress. Empty on old events. + val fingerprint: String = "", ) : EventPayload /** diff --git a/core/events/src/main/kotlin/com/correx/core/events/orchestration/OrchestrationState.kt b/core/events/src/main/kotlin/com/correx/core/events/orchestration/OrchestrationState.kt index ca49d7d5..4f4c4ae9 100644 --- a/core/events/src/main/kotlin/com/correx/core/events/orchestration/OrchestrationState.kt +++ b/core/events/src/main/kotlin/com/correx/core/events/orchestration/OrchestrationState.kt @@ -31,4 +31,11 @@ data class OrchestrationState( // verification→recovery→verification loop must stay bounded across those transitions, exactly as // refinementIterations does for back-edge loops. val recoveryRoutes: Map = emptyMap(), + // Last-seen failure fingerprint per recovering stage id — the recovery-routing analogue of + // gateFailureFingerprints. Lets routeToRecovery tell a no-progress route (same fingerprint, + // charged against recoveryRoutes) from a genuine-progress one (changed fingerprint, free), so a + // recovery that fixes one cause and surfaces the next is not killed by the small route budget. + // Like recoveryRoutes, NOT reset by TransitionExecutedEvent — it must survive the + // verification→recovery→verification round-trip. + val recoveryFailureFingerprints: Map = emptyMap(), ) diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultOrchestrationReducer.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultOrchestrationReducer.kt index 19750f46..d195ab18 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultOrchestrationReducer.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultOrchestrationReducer.kt @@ -95,10 +95,12 @@ class DefaultOrchestrationReducer : OrchestrationReducer { refinementIterations = state.refinementIterations + (p.cycleKey to p.iteration), ) - // Charge the failing stage's recovery-route budget. routeAttempt is the 1-based count the - // orchestrator computed, so fold it directly (idempotent under replay). + // Charge the failing stage's recovery-route budget. routeAttempt is the no-progress count the + // orchestrator computed (progress-aware), so fold it directly (idempotent under replay), and + // record the failure fingerprint the next route compares against. is FailureTicketOpenedEvent -> state.copy( recoveryRoutes = state.recoveryRoutes + (p.stageId.value to p.routeAttempt), + recoveryFailureFingerprints = state.recoveryFailureFingerprints + (p.stageId.value to p.fingerprint), ) else -> state diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultSessionOrchestrator.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultSessionOrchestrator.kt index 58bfb4cc..b8226c19 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultSessionOrchestrator.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/DefaultSessionOrchestrator.kt @@ -36,6 +36,7 @@ import com.correx.core.kernel.execution.WorkflowResult import com.correx.core.kernel.orchestration.subagent.InSessionSubagentRunner import com.correx.core.kernel.orchestration.subagent.SubagentRunRequest import com.correx.core.kernel.orchestration.subagent.SubagentRunner +import com.correx.core.kernel.retry.FailureFingerprint import com.correx.core.kernel.retry.RetryCoordinator import com.correx.core.kernel.retry.RetryDecision import com.correx.core.sessions.Session @@ -298,8 +299,19 @@ class DefaultSessionOrchestrator( state: OrchestrationState, ): StepResult? { val recoveryId = findRecoveryStage(ctx.graph, stageId) ?: return null // no recovery stage: legacy path + // Progress-aware charging (mirrors the per-gate retry budget, design §5): compare this + // failure's fingerprint to the previous route's. A changed fingerprint means the last + // recovery round moved the needle — fixed one cause and surfaced a different one — so the + // route is FREE (budget unchanged) and only the recorded fingerprint advances. Same + // fingerprint = the round changed nothing, so it charges the small route budget. Terminal + // only once the no-progress count is spent; the recovery→origin back-edge refinement guard + // (executeMove, cap = origin stage maxRetries) remains the ultimate loop bound for the + // pathological all-progress case. + val fingerprint = FailureFingerprint.of(reason) + val prev = state.recoveryFailureFingerprints[stageId.value] + val progressed = prev != null && prev != fingerprint val used = state.recoveryRoutes[stageId.value] ?: 0 - if (used >= RECOVERY_ROUTE_BUDGET) { + if (!progressed && used >= RECOVERY_ROUTE_BUDGET) { return StepResult.Terminal( failWorkflow( ctx.sessionId, @@ -309,6 +321,7 @@ class DefaultSessionOrchestrator( ), ) } + val routeAttempt = if (progressed) used else used + 1 emit( ctx.sessionId, FailureTicketOpenedEvent( @@ -319,13 +332,15 @@ class DefaultSessionOrchestrator( requiredCapability = requiredCapability, routeTo = recoveryId, evidence = reason, - routeAttempt = used + 1, + routeAttempt = routeAttempt, + fingerprint = fingerprint, ), ) log.info( - "[Orchestrator] failure ticket session={} stage={} gate={} lacks={} -> route to recovery={} ({}/{})", + "[Orchestrator] failure ticket session={} stage={} gate={} lacks={} -> recovery={} " + + "(charged={}/{}, progressed={})", ctx.sessionId.value, stageId.value, gate, requiredCapability, - recoveryId.value, used + 1, RECOVERY_ROUTE_BUDGET, + recoveryId.value, routeAttempt, RECOVERY_ROUTE_BUDGET, progressed, ) val advancedTo = advanceStage( ctx.sessionId, 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 3936e104..c1704014 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 @@ -1915,6 +1915,11 @@ abstract class SessionOrchestrator( is ToolResult.Success -> { val diff = result.metadata["diff"]?.takeIf { it.isNotBlank() } val affected = fileTool?.affectedPaths(request).orEmpty() + // Read-only tools (file_read, list_dir) have no FileAffectingTool, so `affected` + // is empty — fall back to the raw `path` argument so the receipt still names + // the file/dir the tool looked at (surfaced in the TUI's output row). + val affectedNames = affected.map { it.toString() } + .ifEmpty { listOfNotNull(request.parameters["path"] as? String) } emit( sessionId, ToolExecutionCompletedEvent( @@ -1929,7 +1934,7 @@ abstract class SessionOrchestrator( // Carry the tool's structured metadata (e.g. file_read's contentHash) so // session projections can read it. structuredOutput = result.metadata, - affectedEntities = affected.map { it.toString() }, + affectedEntities = affectedNames, durationMs = 0, tier = tier, timestamp = Clock.System.now(), diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/ListDirTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/ListDirTool.kt index 779ae793..c306fc35 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/ListDirTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/ListDirTool.kt @@ -27,11 +27,15 @@ import kotlin.io.path.name import kotlin.io.path.readText /** - * Recursive, `.gitignore`-aware directory listing — the affordance weak local models should reach - * for instead of shell `ls -R`, which dumps `node_modules`/`build`/`dist` and drowns a small model - * in noise. Read-only (Tier T1, FILE_READ): it shares [FileReadTool]'s path jail and workspace - * anchor. `.gitignore` is honoured for *enumeration only* — the mutation tools deliberately do NOT - * respect it (you legitimately write to ignored paths like `.env`/`dist`). + * `.gitignore`-aware directory listing — the affordance weak local models should reach for instead + * of shell `ls -R`, which dumps `node_modules`/`build`/`dist` and drowns a small model in noise. + * Shallow by default (`recursive=false`): a bare `list_dir .` returns the top-level entries, which + * is what the common question ("what's at the root / does `frontend/` exist?") actually wants — a + * recursive default buried that answer under an alphabetical 400-entry `docs/` subtree flood and + * drove models to re-issue the same call 2-3x (observed: discovery looping on `list_dir .`). Pass + * `recursive:true` to descend. Read-only (Tier T1, FILE_READ): it shares [FileReadTool]'s path jail + * and workspace anchor. `.gitignore` is honoured for *enumeration only* — the mutation tools + * deliberately do NOT respect it (you legitimately write to ignored paths like `.env`/`dist`). */ class ListDirTool( private val allowedPaths: Set = emptySet(), @@ -60,7 +64,7 @@ class ListDirTool( } putJsonObject("recursive") { put("type", "boolean") - put("description", "Descend into sub-directories (gitignored subtrees are pruned). Default true.") + put("description", "Descend into sub-directories (gitignored subtrees are pruned). Default false.") } } put("required", buildJsonArray {}) @@ -90,7 +94,7 @@ class ListDirTool( return@withContext ToolResult.Failure(request.invocationId, it.reason, recoverable = false) } val pathString = (request.parameters["path"] as? String) ?: "." - val recursive = request.parameters["recursive"]?.toString()?.toBoolean() ?: true + val recursive = request.parameters["recursive"]?.toString()?.toBoolean() ?: false val root = resolvePath(pathString) when { !Files.exists(root) -> diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt index b74bfe0a..66a986db 100644 --- a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt @@ -36,7 +36,7 @@ class ListDirToolTest { Files.writeString(root.resolve("package.json"), "{}") val tool = ListDirTool(allowedPaths = setOf(root), workingDir = root) - val out = (tool.execute(request()) as ToolResult.Success).output + val out = (tool.execute(request(recursive = true)) as ToolResult.Success).output assertTrue(out.contains("src/main.ts"), out) assertTrue(out.contains("package.json"), out) @@ -45,6 +45,19 @@ class ListDirToolTest { assertFalse(out.contains("debug.log"), out) } + @Test + fun `default is shallow — a bare list_dir does not descend`(): Unit = runBlocking { + // Regression: a recursive default buried top-level answers ("does frontend/ exist?") under a + // 400-entry alphabetical flood and drove models to re-issue the same list_dir 2-3x. A bare + // call (no recursive param) must now list only immediate children. + val root = Files.createTempDirectory("listdir") + Files.createDirectories(root.resolve("src/deep")).also { Files.writeString(it.resolve("f.ts"), "f") } + val tool = ListDirTool(allowedPaths = setOf(root), workingDir = root) + val out = (tool.execute(request()) as ToolResult.Success).output + assertTrue(out.contains("src/"), out) + assertFalse(out.contains("deep"), out) + } + @Test fun `non-recursive lists only immediate children`(): Unit = runBlocking { val root = Files.createTempDirectory("listdir") diff --git a/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/ExecutionPlanCompiler.kt b/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/ExecutionPlanCompiler.kt index 9f7d531a..0653258f 100644 --- a/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/ExecutionPlanCompiler.kt +++ b/infrastructure/workflow/src/main/kotlin/com/correx/infrastructure/workflow/ExecutionPlanCompiler.kt @@ -5,6 +5,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.BuildExpectation import com.correx.core.transitions.graph.StageConfig import com.correx.core.transitions.graph.TransitionEdge @@ -38,6 +39,16 @@ private const val RECOVERY_PROMPT = // leaves ample headroom for completion.) private const val DEFAULT_STAGE_TOKEN_BUDGET = 16384 +// A compiled freestyle stage must also lift its inference completion cap off StageConfig's 2048 +// default, or the model is truncated (finishReason=length) mid-artifact — and a degenerating local +// model burns the whole 2048 on garbage (e.g. a `<|channel>thought` repetition loop) before it can +// stop. Mirror the static TomlWorkflowLoader path: pin the completion cap to the stage token budget. +private val DEFAULT_STAGE_GENERATION = GenerationConfig( + temperature = 0.7, + topP = 1.0, + maxTokens = DEFAULT_STAGE_TOKEN_BUDGET, +) + class ExecutionPlanCompiler( private val registry: ArtifactKindRegistry, // Names of every registered tool. A stage that references a tool the runtime can't resolve @@ -117,6 +128,7 @@ class ExecutionPlanCompiler( autoBuildGate = s.id == autoGateStageId, semanticReview = s.semanticReview, tokenBudget = DEFAULT_STAGE_TOKEN_BUDGET, + generationConfig = DEFAULT_STAGE_GENERATION, metadata = mapOf("promptInline" to s.prompt), ) } @@ -134,6 +146,7 @@ class ExecutionPlanCompiler( StageId(RECOVERY_STAGE) to StageConfig( allowedTools = setOf("file_write", "shell"), tokenBudget = DEFAULT_STAGE_TOKEN_BUDGET, + generationConfig = DEFAULT_STAGE_GENERATION, metadata = mapOf("role" to "recovery", "promptInline" to RECOVERY_PROMPT), ) } diff --git a/testing/integration/src/test/kotlin/RecoveryRoutingTest.kt b/testing/integration/src/test/kotlin/RecoveryRoutingTest.kt index db8148a5..eb756fd8 100644 --- a/testing/integration/src/test/kotlin/RecoveryRoutingTest.kt +++ b/testing/integration/src/test/kotlin/RecoveryRoutingTest.kt @@ -126,7 +126,10 @@ class RecoveryRoutingTest { override fun all(): List = listOf(tool) } - private fun buildOrchestrator(): Pair { + private fun buildOrchestrator( + staticAnalysis: StaticAnalysisRunner = + StaticAnalysisRunner { _, _ -> StaticAnalysisRunResult(exitCode = 1, output = "TS5023: bad option") }, + ): Pair { val eventStore = InMemoryEventStore() val shell = ShellTool(allowedExecutables = setOf("true"), workingDir = workspace) val toolRegistry = SingleToolRegistry(shell) @@ -141,9 +144,6 @@ class RecoveryRoutingTest { val inferenceRouter = object : InferenceRouter { override suspend fun route(stageId: StageId, requiredCapabilities: Set) = provider } - val failingStaticAnalysis = - StaticAnalysisRunner { _, _ -> StaticAnalysisRunResult(exitCode = 1, output = "TS5023: bad option") } - val repositories = OrchestratorRepositories( eventStore = eventStore, inferenceRepository = InferenceRepository(object : EventReplayer { @@ -172,7 +172,7 @@ class RecoveryRoutingTest { toolExecutor = executor, toolRegistry = toolRegistry, workspacePolicy = WorkspacePolicy(workspace), - staticAnalysisRunner = failingStaticAnalysis, + staticAnalysisRunner = staticAnalysis, approvalEngine = object : ApprovalEngine { override fun evaluate( request: DomainApprovalRequest, @@ -254,4 +254,37 @@ class RecoveryRoutingTest { assertEquals(listOf(1, 2), tickets.map { it.routeAttempt }) assertTrue(events.any { it.payload is WorkflowFailedEvent }) } + + @Test + fun `recovery whose failure keeps changing is not charged the route budget (progress-aware)`(): Unit = runBlocking { + // Each verify run surfaces a DIFFERENT failure (distinct non-digit words so FailureFingerprint, + // which collapses digit runs, actually sees a changed fingerprint). This models a genuine + // multi-cause repair: recovery fixes one cause, the gate now fails on the next. Progress must + // NOT charge the small route budget — so the run routes to recovery MORE than + // RECOVERY_ROUTE_BUDGET times and is instead bounded by the recovery->verify refinement guard. + val variants = listOf("cannot resolve module alpha", "cannot resolve module beta", "cannot resolve module gamma", "cannot resolve module delta", "cannot resolve module epsilon") + var call = 0 + val changingStaticAnalysis = StaticAnalysisRunner { _, _ -> + StaticAnalysisRunResult(exitCode = 1, output = variants[minOf(call++, variants.lastIndex)]) + } + val (orchestrator, eventStore) = buildOrchestrator(changingStaticAnalysis) + val sessionId = SessionId("recovery-progress") + val config = OrchestrationConfig(retryPolicy = RetryPolicy(maxAttempts = 10, backoffMs = 0)) + + val result = orchestrator.run(sessionId, recoveryGraph(), config) + + assertTrue(result is WorkflowResult.Failed, "still terminates — via the refinement guard, not the route budget") + val tickets = eventStore.read(sessionId).mapNotNull { it.payload as? FailureTicketOpenedEvent } + assertTrue(tickets.size > RECOVERY_ROUTE_BUDGET_TEST, "progress routes are free, so more than the budget of routes happen: got ${tickets.size}") + // Every route progressed (changed fingerprint) except the first, so the charged count never + // climbs past its initial 1 — the old unconditional charging would have read [1, 2] then died. + assertEquals(1, tickets.map { it.routeAttempt }.max(), "no-progress charge count must never exceed 1 when every round progresses") + assertTrue(tickets.map { it.fingerprint }.toSet().size == tickets.size, "each route recorded a distinct fingerprint") + } + + private companion object { + // Mirror of DefaultSessionOrchestrator.RECOVERY_ROUTE_BUDGET (private there); kept in sync by the + // `[1, 2]` assertion in the exhaustion test above. + const val RECOVERY_ROUTE_BUDGET_TEST = 2 + } }