diff --git a/apps/server/AGENTS.md b/apps/server/AGENTS.md index bffc26b8..6b2d6eb0 100644 --- a/apps/server/AGENTS.md +++ b/apps/server/AGENTS.md @@ -19,6 +19,7 @@ All sources under `apps/server/src/`. - `GET /health` — health report (probes: event-store, llama-server, disk watermark) - `GET /stats` — metrics report (MetricsProjection) - `GET /metrics/tool-reliability` — per-model tool-call validity across the event log (`ToolReliabilityInspectionService`); groundwork for capability-aware routing +- `GET /metrics/failure-attribution` — terminal-failure attribution across the event log (`FailureAttributionInspectionService`): count and share per `FailureAttribution` layer, the UNKNOWN share, the preserved reasons behind each row, and the `FailureTicketOpened` categories from the same sessions. Read-only: events recorded before `WorkflowFailedEvent.attribution` existed are classified at read time by `FailureAttributor` and reported as `inferred`, never written back. - Optional `[git]` transport creates `run/` from a server-local checkout and pushes it at terminal state; clients review with ordinary Git and never supply a remote URL as `cwd`. - Repo-map L3 embeddings use bounded, recorded source descriptors (module/package, imports, leading purpose comment, symbols); raw file bodies are never embedded. Their versioned `repomap:v2` namespace forces a one-time re-embed when the semantic document format changes. - At boot, `tools.workspace_root` is the authoritative default tool jail. Every session records its own resolved workspace binding; repo maps, project memory, profile/instruction snapshots, and git run branches use that binding and skip unbound sessions. `[project]` never supplies a workspace root. diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/Application.kt b/apps/server/src/main/kotlin/com/correx/apps/server/Application.kt index 4355f037..6344ddb3 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/Application.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/Application.kt @@ -1,6 +1,7 @@ package com.correx.apps.server import com.correx.apps.server.health.HealthInspectionService +import com.correx.apps.server.metrics.FailureAttributionInspectionService import com.correx.apps.server.metrics.ToolReliabilityInspectionService import com.correx.apps.server.routes.providerRoutes import com.correx.apps.server.routes.sessionRoutes @@ -40,6 +41,7 @@ fun Application.configureServer(module: ServerModule) { val globalStreamHandler = GlobalStreamHandler(module) val healthInspection = HealthInspectionService(module.eventStore) val toolReliability = ToolReliabilityInspectionService(module.eventStore) + val failureAttribution = FailureAttributionInspectionService(module.eventStore) routing { get("/health") { @@ -59,6 +61,13 @@ fun Application.configureServer(module: ServerModule) { call.respond(toolReliability.inspect()) } + // Terminal-failure attribution across the whole event log: which layer each WorkflowFailed + // belongs to, and the UNKNOWN share. Read-only; events recorded before the attribution field + // are classified at read time and reported as `inferred`, never written back. + get("/metrics/failure-attribution") { + call.respond(failureAttribution.inspect()) + } + webSocket("/stream") { globalStreamHandler.handle(this) } diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/ServerModule.kt b/apps/server/src/main/kotlin/com/correx/apps/server/ServerModule.kt index 31f37a49..f7f72fcf 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/ServerModule.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/ServerModule.kt @@ -8,6 +8,8 @@ import com.correx.apps.server.registry.ProviderRegistry import com.correx.apps.server.registry.WorkflowRegistry import com.correx.apps.server.workspace.WorkspaceResolver import com.correx.apps.server.workspace.WorkspaceResolution +import com.correx.core.events.events.FailureAttribution +import com.correx.core.events.events.FailureAttributor import com.correx.core.events.events.SessionWorkspaceBoundEvent import com.correx.core.kernel.orchestration.WorkspaceContext import com.correx.core.approvals.ApprovalProjector @@ -478,6 +480,10 @@ class ServerModule( stageId = failingStageId, reason = reason, retryExhausted = retryExhausted, + // This is the catch-all for a throwable that escaped the orchestrator, so the + // default layer is correx itself; the reason text still wins when it names an + // outer layer (provider timeout, missing program, operator cancellation). + attribution = FailureAttributor.classify(reason, fallback = FailureAttribution.HARNESS), ), ), ) diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/freestyle/FreestyleDriver.kt b/apps/server/src/main/kotlin/com/correx/apps/server/freestyle/FreestyleDriver.kt index bbb904e5..a2d3a133 100644 --- a/apps/server/src/main/kotlin/com/correx/apps/server/freestyle/FreestyleDriver.kt +++ b/apps/server/src/main/kotlin/com/correx/apps/server/freestyle/FreestyleDriver.kt @@ -7,6 +7,7 @@ import com.correx.core.events.events.CapabilityGapVerdict import com.correx.core.events.events.EventMetadata import com.correx.core.events.events.ExecutionPlanLockedEvent import com.correx.core.events.events.ExecutionPlanRejectedEvent +import com.correx.core.events.events.FailureAttribution import com.correx.core.events.events.NewEvent import com.correx.core.events.events.PlanGroundingEvaluatedEvent import com.correx.core.events.events.PlanGroundingVerdict @@ -371,6 +372,8 @@ class FreestyleDriver( stageId = StageId("architect"), reason = "execution plan rejected ($source): $reason", retryExhausted = false, + // The rejected plan is the model's own output; the harness evaluated it correctly. + attribution = FailureAttribution.AGENT, ), ), ) diff --git a/apps/server/src/main/kotlin/com/correx/apps/server/metrics/FailureAttributionInspectionService.kt b/apps/server/src/main/kotlin/com/correx/apps/server/metrics/FailureAttributionInspectionService.kt new file mode 100644 index 00000000..1e18782a --- /dev/null +++ b/apps/server/src/main/kotlin/com/correx/apps/server/metrics/FailureAttributionInspectionService.kt @@ -0,0 +1,140 @@ +package com.correx.apps.server.metrics + +import com.correx.core.events.events.FailureAttribution +import com.correx.core.events.events.FailureAttributor +import com.correx.core.events.events.FailureTicketOpenedEvent +import com.correx.core.events.events.WorkflowFailedEvent +import com.correx.core.events.stores.EventStore +import kotlinx.serialization.Serializable + +private const val PERCENT = 100.0 +private const val REASON_BUCKET_MAX = 110 +private const val TOP_REASONS = 10 + +@Serializable +data class AttributionRow( + val attribution: String, + val count: Long, + val sharePct: Double, + /** Failures whose event carried this attribution when it was recorded. */ + val recorded: Long, + /** Failures classified from the preserved reason at read time (the field was absent/UNKNOWN). */ + val inferred: Long, + /** The preserved [WorkflowFailedEvent.reason] texts behind this row, most frequent first. */ + val topReasons: List, + /** Categories of the [FailureTicketOpenedEvent]s in the same sessions — the causal chain when + * more than one layer contributed. Empty when no tickets were opened. */ + val contributingTicketCategories: List, +) + +@Serializable +data class FailureAttributionReport( + val totalFailures: Long, + val recordedFailures: Long, + val inferredFailures: Long, + val unknownCount: Long, + val unknownPct: Double, + val byAttribution: List, + /** Every reason that no marker matched, so UNKNOWN can never sit unexamined. */ + val unknownReasons: List, +) + +/** + * Failure attribution across the whole event log: how many terminal [WorkflowFailedEvent]s belong to + * each layer, and what share is still UNKNOWN. The denominator you need before changing execution + * behaviour — "correx failed N runs" is not actionable, "N of them were harness defects" is. + * + * Read-only and idempotent by construction: historical events recorded before + * [WorkflowFailedEvent.attribution] existed are classified at READ time by [FailureAttributor], the + * same function live emission uses. Nothing is written back — history is append-only, and a + * re-derived classification is a projection, not a fact (Hard Invariant #2). Each row separates what + * was `recorded` at emission from what this service `inferred`, so a backfilled baseline never + * masquerades as originally-recorded data. Re-running it over the same log yields the same numbers. + */ +class FailureAttributionInspectionService(private val eventStore: EventStore) { + + private class Agg { + var recorded: Long = 0 + var inferred: Long = 0 + val reasons: MutableMap = linkedMapOf() + val sessions: MutableSet = linkedSetOf() + } + + @Suppress("NestedBlockDepth") + fun inspect(): FailureAttributionReport { + val byAttribution = linkedMapOf() + val ticketsBySession = linkedMapOf>() + val unknownReasons = linkedMapOf() + + eventStore.allEvents().forEach { stored -> + when (val payload = stored.payload) { + is FailureTicketOpenedEvent -> { + val categories = ticketsBySession.getOrPut(payload.sessionId.value) { linkedMapOf() } + categories[payload.category] = (categories[payload.category] ?: 0) + 1 + } + + is WorkflowFailedEvent -> { + val wasRecorded = payload.attribution != FailureAttribution.UNKNOWN + val attribution = + if (wasRecorded) payload.attribution else FailureAttributor.classify(payload.reason) + val agg = byAttribution.getOrPut(attribution) { Agg() } + if (wasRecorded) agg.recorded++ else agg.inferred++ + val bucket = payload.reason.lineSequence().firstOrNull().orEmpty().take(REASON_BUCKET_MAX) + agg.reasons[bucket] = (agg.reasons[bucket] ?: 0) + 1 + agg.sessions += payload.sessionId.value + if (attribution == FailureAttribution.UNKNOWN) { + unknownReasons[bucket] = (unknownReasons[bucket] ?: 0) + 1 + } + } + + else -> Unit + } + } + + val total = byAttribution.values.sumOf { it.recorded + it.inferred } + val unknown = byAttribution[FailureAttribution.UNKNOWN]?.let { it.recorded + it.inferred } ?: 0 + val rows = byAttribution.entries + .sortedByDescending { it.value.recorded + it.value.inferred } + .map { (attribution, agg) -> row(attribution, agg, total, ticketsBySession) } + + return FailureAttributionReport( + totalFailures = total, + recordedFailures = byAttribution.values.sumOf { it.recorded }, + inferredFailures = byAttribution.values.sumOf { it.inferred }, + unknownCount = unknown, + unknownPct = share(unknown, total), + byAttribution = rows, + unknownReasons = topOf(unknownReasons), + ) + } + + private fun row( + attribution: FailureAttribution, + agg: Agg, + total: Long, + ticketsBySession: Map>, + ): AttributionRow { + val count = agg.recorded + agg.inferred + val tickets = linkedMapOf() + agg.sessions.forEach { sessionId -> + ticketsBySession[sessionId]?.forEach { (category, n) -> + tickets[category] = (tickets[category] ?: 0) + n + } + } + return AttributionRow( + attribution = attribution.name, + count = count, + sharePct = share(count, total), + recorded = agg.recorded, + inferred = agg.inferred, + topReasons = topOf(agg.reasons), + contributingTicketCategories = topOf(tickets), + ) + } + + private fun topOf(counts: Map): List = + counts.entries.sortedByDescending { it.value }.take(TOP_REASONS).map { ReasonCount(it.key, it.value) } + + private fun share(part: Long, total: Long): Double = + if (total == 0L) 0.0 else part.toDouble() / total * PERCENT +} diff --git a/core/events/AGENTS.md b/core/events/AGENTS.md index 5ac3843c..e4217d66 100644 --- a/core/events/AGENTS.md +++ b/core/events/AGENTS.md @@ -20,6 +20,7 @@ CORREX kernel team. This is the most cross-cutting module in the codebase — ch - `JsonEventSerializer` / `EventSerializer` — serialize/deserialize `StoredEvent` to JSON. - `EventDispatcher` — broadcasts events to in-process listeners. - Domain event files: `ApprovalEvents`, `ArtifactEvents`, `ContextEvents`, `InferenceEvents`, `OrchestrationEvents`, `RouterEvents`, `SessionEvents`, `TaskEvents`, `ToolEvents`, `IntentEvents`, `RiskAssessedEvent`, `JournalCompactedEvent`, and many more — all payload definitions live here. +- `FailureAttribution` / `FailureAttributor` — the terminal-failure taxonomy (`AGENT`, `HARNESS`, `WORKFLOW`, `ENVIRONMENT`, `PROVIDER`, `OPERATOR`, `UNKNOWN`) carried as `WorkflowFailedEvent.attribution`, plus the deterministic reason→layer mapping used both at emission and when classifying historical events. One primary attribution per terminal event; a multi-cause chain is the session's `FailureTicketOpenedEvent`s, not a second structure. - `LspDiagnosticsCompletedEvent` records pulled language-server diagnostics or a graceful skip reason; replay consumes this observation and never contacts the server. - Shared vocabulary: `IdentityTypes` (SessionId, TaskId, etc.), `Tier`, `TokenUsage`, `ToolReceipt`, `ToolRequest`, `RiskLevel`, `RetryPolicy`, `GrantScope`, `GrantLedger`. @@ -30,7 +31,8 @@ CORREX kernel team. This is the most cross-cutting module in the codebase — ch - Event classes are `@Serializable data class` with no mutable state. No methods beyond data accessors. - `RunBranchPushedEvent` records an optional server Git transport push only after it succeeds; its branch/base/head SHAs are observations, not values replay recalculates. - `RepoMapEntry.descriptor` is a bounded source-purpose observation recorded with the repo map and used when constructing semantic L3 embeddings. -- Do not add domain logic to events. They are records, not actors. +- Do not add domain logic to events. They are records, not actors. `FailureAttributor` is the one exception by design: a pure reason→layer function that must be identical for live emission and for historical classification, so it lives beside the enum it returns. +- `WorkflowFailedEvent.attribution` defaults to `UNKNOWN` so pre-field events replay unchanged. Classify those at READ time (see `FailureAttributionInspectionService`); never rewrite history to backfill them. - `EgressAllowlistProjection` — special projection kept in this module because it is used by both `core:toolintent` and `core:events` consumers; it is a shared cross-cutting projection. ## Verification diff --git a/core/events/src/main/kotlin/com/correx/core/events/events/FailureAttribution.kt b/core/events/src/main/kotlin/com/correx/core/events/events/FailureAttribution.kt new file mode 100644 index 00000000..c520e987 --- /dev/null +++ b/core/events/src/main/kotlin/com/correx/core/events/events/FailureAttribution.kt @@ -0,0 +1,142 @@ +package com.correx.core.events.events + +import kotlinx.serialization.Serializable + +/** + * WHOSE failure a terminal [WorkflowFailedEvent] was: the primary layer that has to change for the + * run to succeed. One value per terminal event. When several causes contributed, the causal chain is + * the session's [FailureTicketOpenedEvent]s — this enum does not model chains. + * + * The point is measurement: "correx failed 99 runs" is not actionable, "61 of them were harness + * defects" is. Read the layer, not the symptom. + */ +@Serializable +enum class FailureAttribution { + /** The model produced invalid work while the harness operated correctly. */ + AGENT, + + /** Correx's own runtime: a linkage error, a bug in a reducer/tool layer, a false observation + * handed to the agent, or an expectation correx could not evaluate for lack of instrumentation. */ + HARNESS, + + /** The workflow/graph definition: no transition matched, a condition referenced a field that + * cannot exist, a declared prompt or stage was never authored. */ + WORKFLOW, + + /** The machine the run executes on: a missing executable, permissions, disk, ports. */ + ENVIRONMENT, + + /** The inference provider: unavailable, timed out, or answered with a body correx cannot read. */ + PROVIDER, + + /** A human ended the run: cancellation, or a denied/rejected approval. */ + OPERATOR, + + /** Not classifiable from the recorded reason. A metric, not a bucket: a rising UNKNOWN share + * means the taxonomy or the reason text needs work, and every UNKNOWN is a defect to triage. */ + UNKNOWN, +} + +/** + * Deterministic mapping from a terminal failure reason to its [FailureAttribution]. + * + * Same function for live emission and for classifying historical events recorded before the field + * existed, so a backfilled baseline and a live metric are the same measurement. It is a pure + * function of the reason string: no clock, no I/O, no session lookup — safe to re-run over the whole + * event log any number of times. + * + * Markers are matched in layer order — OPERATOR, PROVIDER, ENVIRONMENT, WORKFLOW, HARNESS, AGENT — + * because the outermost cause wins: a provider timeout that surfaces as an artifact-validation + * failure is still a provider failure. Match on what the layer says about ITSELF (a provider being + * unavailable, a program that cannot be run), never on the domain of the run: nothing here may key + * on a language, framework, build tool or task type. + */ +object FailureAttributor { + + /** + * Classifies [reason]. [fallback] is returned when no marker matches — a call site that knows + * the layer from its position (e.g. a top-level catch-all in the correx runtime) supplies its + * own instead of leaving the failure [FailureAttribution.UNKNOWN]. + */ + fun classify(reason: String, fallback: FailureAttribution = FailureAttribution.UNKNOWN): FailureAttribution { + val text = reason.lowercase() + // A reason that is a bare JVM binary name with no prose is a linkage/classload error + // (NoClassDefFoundError.getMessage()), i.e. a correx runtime defect. + if (text.isNotBlank() && !text.contains(' ') && text.contains('/') && !text.contains('.')) { + return FailureAttribution.HARNESS + } + return MARKERS.firstOrNull { (_, markers) -> markers.any { it in text } }?.first ?: fallback + } + + private val MARKERS: List>> = listOf( + FailureAttribution.OPERATOR to listOf( + "cancelled", + "canceled", + "approval denied", + "approval rejected", + "rejected by operator", + ), + FailureAttribution.PROVIDER to listOf( + "is unavailable", + "health check failed", + "connection refused", + "request timeout has expired", + "no provider satisfies", + "returned 400", + "returned 401", + "returned 403", + "returned 404", + "returned 5", + "chatcompletionresponse", + "no completion returned", + "context window exceeded", + ), + FailureAttribution.ENVIRONMENT to listOf( + "cannot run program", + "exec failed", + "command not found", + "permission denied", + "no space left", + "address already in use", + ), + FailureAttribution.WORKFLOW to listOf( + "no transition condition matched", + "no matching transition", + "condition evaluation failed", + // A stage's declaration disagrees with reality: the prerequisite it names is unresolved + // or sits outside the scope it declared. Both are authoring defects in the definition. + "build prerequisite", + "declared prompt", + "unknown stage", + "no such stage", + ), + FailureAttribution.HARNESS to listOf( + "noclassdeffounderror", + "nosuchmethod", + "classnotfound", + "not supported in map", + "hex string must have even length", + "could not be evaluated", + "no instrumentation", + "unexpected orchestrator failure", + ), + FailureAttribution.AGENT to listOf( + "did not produce declared artifacts", + "did not satisfy its file contract", + "did not pass", + "failed semantic review", + "declared no artifacts", + "review loop exhausted", + // A call plane-2 denied: the harness evaluated policy correctly, the agent proposed it. + "blocked by tool-call policy", + "validation failed", + "artifact repair failed", + "repair ladder exhausted", + "recovery route budget exhausted", + "refinement loop", + "execution plan rejected", + "is stuck", + "failed to decode", + ), + ) +} 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 5f403458..ad59b35b 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 @@ -34,6 +34,12 @@ data class WorkflowFailedEvent( val stageId: StageId, val reason: String, val retryExhausted: Boolean, + // WHOSE failure this was — the layer that must change for the run to succeed (see + // [FailureAttribution]). Set at emission by the site that knows the cause, or derived from + // [reason] by [FailureAttributor]; the original [reason] is always preserved alongside it. + // Defaulted to UNKNOWN so events recorded before this field replay unchanged: a classification + // for those is INFERRED at read time, never written back over history. + val attribution: FailureAttribution = FailureAttribution.UNKNOWN, ) : EventPayload /** diff --git a/core/events/src/test/kotlin/com/correx/core/events/events/FailureAttributionTest.kt b/core/events/src/test/kotlin/com/correx/core/events/events/FailureAttributionTest.kt new file mode 100644 index 00000000..47530ac7 --- /dev/null +++ b/core/events/src/test/kotlin/com/correx/core/events/events/FailureAttributionTest.kt @@ -0,0 +1,140 @@ +package com.correx.core.events.events + +import com.correx.core.events.types.SessionId +import com.correx.core.events.types.StageId +import kotlinx.serialization.json.Json +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * The taxonomy's contract. Cases are drawn from real `WorkflowFailed.reason` texts in the local + * event log so the mapping is checked against failures that actually happened, and every case keys + * on what a LAYER says about itself — never on a language, framework or build tool. + */ +class FailureAttributionTest { + + private fun assertLayer(expected: FailureAttribution, reason: String) = + assertEquals(expected, FailureAttributor.classify(reason), reason) + + @Test + fun `operator-ended runs`() { + assertLayer(FailureAttribution.OPERATOR, "CANCELLED") + assertLayer(FailureAttribution.OPERATOR, "approval denied") + assertLayer(FailureAttribution.OPERATOR, "approval rejected for stage architect") + } + + @Test + fun `provider failures`() { + assertLayer( + FailureAttribution.PROVIDER, + "Provider 'llama-cpp:default' is unavailable: Health check failed: Connection refused", + ) + assertLayer( + FailureAttribution.PROVIDER, + "Request timeout has expired [url=http://127.0.0.1:10000/v1/chat/completions, " + + "request_timeout=600000 ms]", + ) + assertLayer(FailureAttribution.PROVIDER, "No provider satisfies capabilities [] for stage 'routing'") + // A provider body correx cannot decode is a provider-communication failure, not a bad artifact. + assertLayer( + FailureAttribution.PROVIDER, + "Illegal input: Fields [id, choices, usage] are required for type with serial name " + + "'com.correx.infrastructure.inference.llama.cpp.ChatCompletionResponse', but they were missing", + ) + assertLayer(FailureAttribution.PROVIDER, "llama-server returned 400 Bad Request: {\"error\":{}}") + } + + @Test + fun `environment failures`() { + assertLayer( + FailureAttribution.ENVIRONMENT, + "Cannot run program \"cd\" (in directory \"/w\"): Exec failed, error: 2 (No such file or directory)", + ) + } + + @Test + fun `workflow-definition failures`() { + assertLayer(FailureAttribution.WORKFLOW, "no transition condition matched from stage analyst") + assertLayer( + FailureAttribution.WORKFLOW, + "condition evaluation failed on 'verify_completion->done': Field 'verdict' not found", + ) + assertLayer(FailureAttribution.WORKFLOW, "[SessionOrchestrator] stage=analyst: declared prompt 'x' missing") + assertLayer(FailureAttribution.WORKFLOW, "no matching transition from stage A") + assertLayer(FailureAttribution.WORKFLOW, "build prerequisite 'x' unresolved after bootstrap: missing") + } + + @Test + fun `harness failures`() { + // A bare JVM binary name with no prose is a linkage error inside correx itself. + assertLayer( + FailureAttribution.HARNESS, + "com/correx/core/kernel/orchestration/SessionOrchestrator\$failWorkflow\$1", + ) + assertLayer(FailureAttribution.HARNESS, "com/correx/core/approvals/GrantLedgerKt") + assertLayer(FailureAttribution.HARNESS, "null values are not supported in Map") + // An expectation correx could not evaluate for lack of instrumentation is ours, not the agent's. + assertLayer(FailureAttribution.HARNESS, "expected_result could not be evaluated: no instrumentation") + } + + @Test + fun `agent failures`() { + assertLayer(FailureAttribution.AGENT, "stage implementer did not produce declared artifacts: patch") + assertLayer(FailureAttribution.AGENT, "validation failed") + assertLayer(FailureAttribution.AGENT, "artifact repair failed (FORMATTING): could not extract a JSON object") + assertLayer(FailureAttribution.AGENT, "refinement loop 'implementer->reviewer' exceeded 2 iterations") + assertLayer(FailureAttribution.AGENT, "recovery route budget exhausted for stage ui_review (gate=execution)") + assertLayer(FailureAttribution.AGENT, "repair ladder exhausted for stage x (gate=stage_loop_break)") + assertLayer(FailureAttribution.AGENT, "execution plan rejected (grounding): plan failed grounding") + assertLayer(FailureAttribution.AGENT, "stage x did not satisfy its file contract. Fix these before review:") + assertLayer(FailureAttribution.AGENT, "stage x did not pass its PROJECT build gate") + assertLayer(FailureAttribution.AGENT, "stage x did not pass static analysis. Fix these before review:") + assertLayer(FailureAttribution.AGENT, "stage x failed semantic review — fix these correctness issues:") + assertLayer(FailureAttribution.AGENT, "stage x declared no artifacts and ran no tools") + assertLayer(FailureAttribution.AGENT, "review loop exhausted after exactly 3 cycles.") + assertLayer(FailureAttribution.AGENT, "blocked by tool-call policy") + } + + @Test + fun `an unmatched reason is UNKNOWN, and a call site may supply its own fallback`() { + assertLayer(FailureAttribution.UNKNOWN, "something nobody has seen before") + assertEquals( + FailureAttribution.HARNESS, + FailureAttributor.classify("something nobody has seen before", FailureAttribution.HARNESS), + ) + // The reason text still wins over a call site's fallback when it names an outer layer. + assertEquals( + FailureAttribution.OPERATOR, + FailureAttributor.classify("CANCELLED", FailureAttribution.HARNESS), + ) + } + + @Test + fun `classification is a pure function of the reason`() { + val reason = "no transition condition matched from stage analyst" + assertEquals(FailureAttributor.classify(reason), FailureAttributor.classify(reason)) + } + + @Test + fun `an event recorded before the field replays as UNKNOWN with its reason preserved`() { + val stored = """{"sessionId":"s","stageId":"st","reason":"CANCELLED","retryExhausted":false}""" + val event = Json.decodeFromString(stored) + assertEquals(FailureAttribution.UNKNOWN, event.attribution) + assertEquals("CANCELLED", event.reason) + // …and the historical baseline classifies it at read time, without rewriting history. + assertEquals(FailureAttribution.OPERATOR, FailureAttributor.classify(event.reason)) + } + + @Test + fun `a live event carries its attribution through a round-trip`() { + val event = WorkflowFailedEvent( + sessionId = SessionId("s"), + stageId = StageId("st"), + reason = "no transition condition matched from stage analyst", + retryExhausted = true, + attribution = FailureAttribution.WORKFLOW, + ) + val json = Json.encodeToString(WorkflowFailedEvent.serializer(), event) + assertEquals(event, Json.decodeFromString(WorkflowFailedEvent.serializer(), json)) + } +} diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt index 2fc4c07d..62d43022 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorPreview.kt @@ -1,4 +1,5 @@ package com.correx.core.kernel.orchestration +import com.correx.core.tools.contract.ToolPath import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.serialization.json.Json @@ -46,8 +47,7 @@ internal suspend fun readFileIfExists(path: String, workspaceRoot: java.nio.file // Resolve relative paths against the session's workspace root, same as the tools do — // resolving against the daemon CWD showed the operator the wrong file (or nothing) when // server CWD ≠ workspace_root. - val raw = java.nio.file.Paths.get(path) - val filePath = if (raw.isAbsolute || workspaceRoot == null) raw else workspaceRoot.resolve(raw) + val filePath = ToolPath.resolve(path, workspaceRoot) if (java.nio.file.Files.exists(filePath)) { java.nio.file.Files.readString(filePath) } else null diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkflow.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkflow.kt index d083a4a6..810ffd61 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkflow.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkflow.kt @@ -5,6 +5,8 @@ import com.correx.core.events.events.CritiqueFindingsRecordedEvent import com.correx.core.events.events.CritiqueOutcomeCorrelatedEvent import com.correx.core.events.events.EventMetadata import com.correx.core.events.events.EventPayload +import com.correx.core.events.events.FailureAttribution +import com.correx.core.events.events.FailureAttributor import com.correx.core.events.events.NewEvent import com.correx.core.events.events.TransitionExecutedEvent import com.correx.core.events.events.WorkflowCompletedEvent @@ -108,6 +110,9 @@ internal suspend fun SessionOrchestrator.failWorkflow( stageId: StageId, reason: String, retryExhausted: Boolean, + // Null ⇒ derive the attribution from [reason] (FailureAttributor). A caller that knows the layer + // from its own position passes it explicitly instead of relying on the reason text. + attribution: FailureAttribution? = null, ): WorkflowResult.Failed { log.warn( "[Orchestrator] FAILED session={} stage={} reason={} retryExhausted={}", @@ -134,7 +139,16 @@ internal suspend fun SessionOrchestrator.failWorkflow( // (e.g. a serialization edge), we cannot record the failure at all — log it loudly with the // full throwable so it is never silent, then still return a clean Failed result. runCatching { - emit(sessionId, WorkflowFailedEvent(sessionId, stageId, reason, retryExhausted)) + emit( + sessionId, + WorkflowFailedEvent( + sessionId, + stageId, + reason, + retryExhausted, + attribution ?: FailureAttributor.classify(reason), + ), + ) }.onFailure { e -> log.error( "[Orchestrator] failWorkflow: FAILED to record terminal WorkflowFailedEvent — event " + diff --git a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt index 9a1a6eef..ee47c922 100644 --- a/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt +++ b/core/kernel/src/main/kotlin/com/correx/core/kernel/orchestration/SessionOrchestratorWorkspace.kt @@ -1,5 +1,6 @@ package com.correx.core.kernel.orchestration +import com.correx.core.events.events.FailureAttribution import com.correx.core.events.events.ToolCallAssessedEvent import com.correx.core.events.events.RepoMapComputedEvent import com.correx.core.events.events.OutsidePathAccessGrantedEvent @@ -212,7 +213,16 @@ internal suspend fun SessionOrchestrator.handleCancellation( stageId: StageId, ): WorkflowResult.Cancelled { log.warn("[Orchestrator] CANCELLED session={} stage={}", sessionId.value, stageId.value) - emit(sessionId, WorkflowFailedEvent(sessionId, stageId, "CANCELLED", retryExhausted = false)) + emit( + sessionId, + WorkflowFailedEvent( + sessionId, + stageId, + "CANCELLED", + retryExhausted = false, + attribution = FailureAttribution.OPERATOR, + ), + ) cancellations.remove(sessionId) return WorkflowResult.Cancelled(sessionId) } diff --git a/core/toolintent/AGENTS.md b/core/toolintent/AGENTS.md index ca72f1c7..b759643d 100644 --- a/core/toolintent/AGENTS.md +++ b/core/toolintent/AGENTS.md @@ -24,7 +24,7 @@ CORREX kernel team. This module enforces Hard Invariant #9 for the tool-call pat - `WorkspacePolicy` — aggregates rules and configuration for a workspace. - `WorldProbe` — performs environment checks (filesystem, network) and records the observations as events immediately (Hard Invariant #9). Never call `WorldProbe` during replay. - `EgressAllowlist` — current egress allowlist; rebuilt from `EgressAllowlistProjection` (in `core:events`). -- `ParamValueExtractor` — extracts typed parameter values from tool call arguments. +- `ParamValueExtractor` — extracts typed parameter values from tool call arguments. `candidatePathStrings` = every path-like argument (`ParamRole.PATH` + `ParamRole.SOURCE_PATH`), used by the containment/existence gates; `writeTargetPathStrings` = only the paths a call MUTATES (`ParamRole.PATH`), used by the write-target gates. A tool declaring none of those roles falls back to sniffing path-like strings, so `shell` is unaffected. - `RiskMapping` — maps rule violations to risk levels for `core:risk`. - `SessionContext` — session-scoped context passed to rules during evaluation. @@ -33,6 +33,8 @@ CORREX kernel team. This module enforces Hard Invariant #9 for the tool-call pat - Hard Invariant #9: all `WorldProbe` calls record observations as events. Replay reads those recorded events — it must not call `WorldProbe` again. - Hard Invariant #5: every tool call must be assessed before execution. Assessment result is recorded as `ToolCallAssessmentEvents` in `core:events`. - New rules implement `ToolCallRule` and are registered in `WorkspacePolicy`. Do not add rule logic directly to `ToolCallAssessor`. +- Resolve every model-supplied path through `ToolPath.resolve` (`core:tools`) — the one canonical normalization rule, shared with the filesystem tools. A rule that resolves paths itself will judge a different path than the tool operates on (the `~` bug: `~/x` resolved to `/~/x`, so a real home-directory file was reported as a non-existent in-workspace file and the out-of-workspace prompt never fired). +- A call that declares a `SOURCE_PATH` takes its content from disk, not from the model, so `ReadBeforeWriteRule` exempts it: requiring a read of a copied file's bytes is unsatisfiable for binaries and defeats the purpose of the tool. ## Verification diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt index 57ef6242..d5bdf1f9 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ManifestContainmentRule.kt @@ -7,6 +7,7 @@ import com.correx.core.toolintent.ToolCallAssessmentInput import com.correx.core.toolintent.ToolCallRule import com.correx.core.toolintent.maxAction import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolPath import com.correx.core.validation.model.ValidationIssue import com.correx.core.validation.model.ValidationSeverity import java.nio.file.FileSystems @@ -52,10 +53,8 @@ class ManifestContainmentRule : ToolCallRule { val observations = mutableListOf() var disposition = RiskAction.PROCEED - for (raw in candidatePathStrings(input.paramRoles, input.request.parameters)) { - val candidate = Path.of(raw) - val resolvedInput = - if (candidate.isAbsolute) candidate else input.workspace.workspaceRoot.resolve(candidate) + for (raw in writeTargetPathStrings(input.paramRoles, input.request.parameters)) { + val resolvedInput = ToolPath.resolve(raw, input.workspace.workspaceRoot) val resolvedReal = input.probe.resolveReal(resolvedInput) val inWorkspace = resolvedReal.startsWith(workspaceReal) val relative = if (inWorkspace) workspaceReal.relativize(resolvedReal).toString() else raw diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ParamValueExtractor.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ParamValueExtractor.kt index c8867618..08d0978d 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ParamValueExtractor.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ParamValueExtractor.kt @@ -14,13 +14,31 @@ internal fun extractParamStrings(value: Any?): List = when (value) { } /** - * The path-like argument strings of a tool call: the values of params declared - * [ParamRole.PATH], or — when none are declared — any string value that looks like a - * path. Shared by the path-containment and write-manifest rules so both judge exactly - * the same set of targets. + * Every path-like argument of a tool call — the call's own targets ([ParamRole.PATH]) and any path + * it merely reads from ([ParamRole.SOURCE_PATH]). Used by the containment/existence gates, which + * must judge both: a source outside the workspace is as much an escape as a destination outside it. */ -internal fun candidatePathStrings(paramRoles: Map, parameters: Map): List { - val declared = paramRoles.filterValues { it == ParamRole.PATH }.keys +internal fun candidatePathStrings(paramRoles: Map, parameters: Map): List = + pathStringsForRoles(paramRoles, parameters, setOf(ParamRole.PATH, ParamRole.SOURCE_PATH)) + +/** + * Only the paths a tool call MUTATES ([ParamRole.PATH]). Used by the write-target gates + * (read-before-write, stale-write, write scope, write manifest): blocking a copy because its + * SOURCE was never read, or charging a source against the task's write scope, would be wrong. + */ +internal fun writeTargetPathStrings(paramRoles: Map, parameters: Map): List = + pathStringsForRoles(paramRoles, parameters, setOf(ParamRole.PATH)) + +/** + * Values of the params declared with one of [roles] or — when the tool declares none of them (e.g. + * `shell`, whose param is an [ParamRole.EXEC_COMMAND]) — any string value that looks like a path. + */ +private fun pathStringsForRoles( + paramRoles: Map, + parameters: Map, + roles: Set, +): List { + val declared = paramRoles.filterValues { it in roles }.keys return if (declared.isNotEmpty()) { declared.flatMap { extractParamStrings(parameters[it]) } } else { diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/PathContainmentRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/PathContainmentRule.kt index 52a35c7a..941cce11 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/PathContainmentRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/PathContainmentRule.kt @@ -7,9 +7,9 @@ import com.correx.core.toolintent.ToolCallAssessmentInput import com.correx.core.toolintent.ToolCallRule import com.correx.core.toolintent.maxAction import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolPath import com.correx.core.validation.model.ValidationIssue import com.correx.core.validation.model.ValidationSeverity -import java.nio.file.Path /** * Effect-based path containment. Dispatches on FILE_READ / FILE_WRITE. For every @@ -34,9 +34,7 @@ class PathContainmentRule : ToolCallRule { var disposition = RiskAction.PROCEED for (raw in candidatePathStrings(input.paramRoles, input.request.parameters)) { - val candidate = Path.of(raw) - val resolvedInput = - if (candidate.isAbsolute) candidate else input.workspace.workspaceRoot.resolve(candidate) + val resolvedInput = ToolPath.resolve(raw, input.workspace.workspaceRoot) val resolvedReal = input.probe.resolveReal(resolvedInput) val exists = input.probe.exists(resolvedInput) val privileged = privilegedReal.any { resolvedReal.startsWith(it) } diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReadBeforeWriteRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReadBeforeWriteRule.kt index 53a81ed1..08cc8e7d 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReadBeforeWriteRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReadBeforeWriteRule.kt @@ -6,7 +6,9 @@ import com.correx.core.toolintent.ToolCallAssessment import com.correx.core.toolintent.ToolCallAssessmentInput import com.correx.core.toolintent.ToolCallRule import com.correx.core.toolintent.maxAction +import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolPath import com.correx.core.validation.model.ValidationIssue import com.correx.core.validation.model.ValidationSeverity import java.nio.file.Path @@ -27,6 +29,12 @@ class ReadBeforeWriteRule : ToolCallRule { ToolCapability.FILE_WRITE in capabilities override fun assess(input: ToolCallAssessmentInput): ToolCallAssessment { + // A call that declares a SOURCE_PATH takes its content from a file on disk, not from the + // model's memory, so there is nothing for this gate to protect: demanding a file_read of the + // source (or of the target being replaced by it) would force the very bytes-through-context + // round-trip that such tools exist to avoid — and is unsatisfiable for a binary file. + if (input.paramRoles.containsValue(ParamRole.SOURCE_PATH)) return ToolCallAssessment() + val root = input.workspace.workspaceRoot val readReal = input.session.reads.map { realOf(input, root, it) }.toSet() @@ -34,7 +42,7 @@ class ReadBeforeWriteRule : ToolCallRule { val observations = mutableListOf() var disposition = RiskAction.PROCEED - for (raw in candidatePathStrings(input.paramRoles, input.request.parameters)) { + for (raw in writeTargetPathStrings(input.paramRoles, input.request.parameters)) { val resolvedInput = resolveInput(root, raw) val exists = input.probe.exists(resolvedInput) val read = input.probe.resolveReal(resolvedInput) in readReal @@ -58,10 +66,7 @@ class ReadBeforeWriteRule : ToolCallRule { return ToolCallAssessment(issues = issues, observations = observations, disposition = disposition) } - private fun resolveInput(root: Path, raw: String): Path { - val candidate = Path.of(raw) - return if (candidate.isAbsolute) candidate else root.resolve(candidate) - } + private fun resolveInput(root: Path, raw: String): Path = ToolPath.resolve(raw, root) private fun realOf(input: ToolCallAssessmentInput, root: Path, raw: String): Path = input.probe.resolveReal(resolveInput(root, raw)) diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReferenceExistsRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReferenceExistsRule.kt index 6192a13d..544dba91 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReferenceExistsRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/ReferenceExistsRule.kt @@ -7,9 +7,9 @@ import com.correx.core.toolintent.ToolCallAssessmentInput import com.correx.core.toolintent.ToolCallRule import com.correx.core.toolintent.maxAction import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolPath import com.correx.core.validation.model.ValidationIssue import com.correx.core.validation.model.ValidationSeverity -import java.nio.file.Path /** * Reference-must-exist gate (anti-hallucination). Dispatches on FILE_READ: a read of a path that is @@ -37,8 +37,7 @@ class ReferenceExistsRule : ToolCallRule { var disposition = RiskAction.PROCEED for (raw in candidatePathStrings(input.paramRoles, input.request.parameters)) { - val candidate = Path.of(raw) - val resolvedInput = if (candidate.isAbsolute) candidate else root.resolve(candidate) + val resolvedInput = ToolPath.resolve(raw, root) val exists = input.probe.exists(resolvedInput) val inWorkspace = input.probe.resolveReal(resolvedInput).startsWith(workspaceReal) diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/StaleWriteRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/StaleWriteRule.kt index 880cbd6a..f096b69f 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/StaleWriteRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/StaleWriteRule.kt @@ -7,6 +7,7 @@ import com.correx.core.toolintent.ToolCallAssessmentInput import com.correx.core.toolintent.ToolCallRule import com.correx.core.toolintent.maxAction import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolPath import com.correx.core.validation.model.ValidationIssue import com.correx.core.validation.model.ValidationSeverity import java.nio.file.Path @@ -35,7 +36,7 @@ class StaleWriteRule : ToolCallRule { val observations = mutableListOf() var disposition = RiskAction.PROCEED - for (raw in candidatePathStrings(input.paramRoles, input.request.parameters)) { + for (raw in writeTargetPathStrings(input.paramRoles, input.request.parameters)) { val resolved = resolveInput(root, raw) val real = input.probe.resolveReal(resolved) val recorded = hashByReal[real] @@ -60,10 +61,7 @@ class StaleWriteRule : ToolCallRule { return ToolCallAssessment(issues = issues, observations = observations, disposition = disposition) } - private fun resolveInput(root: Path, raw: String): Path { - val candidate = Path.of(raw) - return if (candidate.isAbsolute) candidate else root.resolve(candidate) - } + private fun resolveInput(root: Path, raw: String): Path = ToolPath.resolve(raw, root) private fun realOf(input: ToolCallAssessmentInput, root: Path, raw: String): Path = input.probe.resolveReal(resolveInput(root, raw)) diff --git a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt index b1729845..e4f23296 100644 --- a/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt +++ b/core/toolintent/src/main/kotlin/com/correx/core/toolintent/rules/WriteScopeRule.kt @@ -7,10 +7,10 @@ import com.correx.core.toolintent.ToolCallAssessmentInput import com.correx.core.toolintent.ToolCallRule import com.correx.core.toolintent.maxAction import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolPath import com.correx.core.validation.model.ValidationIssue import com.correx.core.validation.model.ValidationSeverity import java.nio.file.FileSystems -import java.nio.file.Path /** * Write-scope adherence. When the session has claimed a task that declared affected_paths, an @@ -37,11 +37,8 @@ class WriteScopeRule : ToolCallRule { val observations = mutableListOf() var disposition = RiskAction.PROCEED - for (raw in candidatePathStrings(input.paramRoles, input.request.parameters)) { - val candidate = Path.of(raw) - val resolvedReal = input.probe.resolveReal( - if (candidate.isAbsolute) candidate else input.workspace.workspaceRoot.resolve(candidate), - ) + for (raw in writeTargetPathStrings(input.paramRoles, input.request.parameters)) { + val resolvedReal = input.probe.resolveReal(ToolPath.resolve(raw, input.workspace.workspaceRoot)) if (!resolvedReal.startsWith(workspaceReal)) continue // out of workspace: not this gate val rel = workspaceReal.relativize(resolvedReal) val inScope = matchers.any { it.matches(rel) } diff --git a/core/toolintent/src/test/kotlin/com/correx/core/toolintent/PathNormalizationRuleTest.kt b/core/toolintent/src/test/kotlin/com/correx/core/toolintent/PathNormalizationRuleTest.kt new file mode 100644 index 00000000..1b3ec754 --- /dev/null +++ b/core/toolintent/src/test/kotlin/com/correx/core/toolintent/PathNormalizationRuleTest.kt @@ -0,0 +1,149 @@ +package com.correx.core.toolintent + +import com.correx.core.events.events.ToolRequest +import com.correx.core.events.risk.RiskAction +import com.correx.core.events.types.SessionId +import com.correx.core.events.types.StageId +import com.correx.core.events.types.ToolInvocationId +import com.correx.core.toolintent.rules.PathContainmentRule +import com.correx.core.toolintent.rules.ReadBeforeWriteRule +import com.correx.core.toolintent.rules.ReferenceExistsRule +import com.correx.core.toolintent.rules.WriteScopeRule +import com.correx.core.tools.contract.ParamRole +import com.correx.core.tools.contract.ToolCapability +import java.nio.file.Path +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Plane-2 must judge the SAME path the tool will operate on (ToolPath), and must judge a call's + * source path differently from its write target. + * + * The `~` cases pin the harness bug from the 2026-08 web-ui runs: a correct diagnosis of + * `~/.gradle/init.d/offline.gradle` was resolved to `/~/.gradle/…`, so the reference gate + * called a real file a hallucination and the out-of-workspace prompt never fired. + */ +class PathNormalizationRuleTest { + + private val workspace = Path.of("/work/project") + private val home: String = System.getProperty("user.home") + private val tildeTarget = "~/.gradle/init.d/offline.gradle" + + private class FakeProbe(private val existing: Set = emptySet()) : WorldProbe { + override fun exists(path: Path): Boolean = path.toAbsolutePath().normalize() in existing + override fun resolveReal(path: Path): Path = path.toAbsolutePath().normalize() + } + + private fun input( + parameters: Map, + capabilities: Set, + probe: WorldProbe, + paramRoles: Map = emptyMap(), + reads: Set = emptySet(), + tool: String = "file_read", + activeTask: ActiveTask? = null, + ) = ToolCallAssessmentInput( + request = ToolRequest(ToolInvocationId("i"), SessionId("s"), StageId("st"), tool, parameters), + capabilities = capabilities, + workspace = WorkspacePolicy(workspace, emptyList()), + probe = probe, + paramRoles = paramRoles, + session = SessionContext(reads = reads, activeTask = activeTask), + ) + + @Test + fun `a tilde path is not mistaken for a non-existent in-workspace file`() { + val real = Path.of(home, ".gradle/init.d/offline.gradle") + val r = ReferenceExistsRule().assess( + input( + mapOf("path" to tildeTarget), + setOf(ToolCapability.FILE_READ), + FakeProbe(existing = setOf(real)), + ), + ) + assertEquals(RiskAction.PROCEED, r.disposition) + assertTrue(r.issues.isEmpty(), "a real home-directory file must not be reported as a hallucination") + assertEquals("true", r.observations.single().facts["exists"]) + assertEquals("false", r.observations.single().facts["inWorkspace"]) + } + + @Test + fun `a tilde path still prompts as an out-of-workspace read`() { + // Containment behaviour is preserved: expansion moves the path OUT of the workspace, which is + // where it always belonged, so the operator approval fires instead of being silently skipped. + val r = PathContainmentRule().assess( + input(mapOf("path" to tildeTarget), setOf(ToolCapability.FILE_READ), FakeProbe()), + ) + assertEquals(RiskAction.PROMPT_USER, r.disposition) + assertEquals("PATH_OUTSIDE_WORKSPACE", r.issues.single().code) + assertEquals(Path.of(home, ".gradle/init.d/offline.gradle").toString(), r.observations.single().facts["resolved"]) + } + + @Test + fun `containment judges a source path as well as a write target`() { + val r = PathContainmentRule().assess( + input( + mapOf("source" to "/etc/hosts", "dest" to "public/hosts"), + setOf(ToolCapability.FILE_WRITE), + FakeProbe(), + paramRoles = mapOf("source" to ParamRole.SOURCE_PATH, "dest" to ParamRole.PATH), + tool = "file_copy", + ), + ) + assertEquals(RiskAction.PROMPT_USER, r.disposition) + assertEquals(2, r.observations.size) + } + + @Test + fun `the write-scope gate judges the write target only, not the source`() { + // A source path is read, not mutated, so it is not charged against the task's write scope — + // otherwise every copy of an in-repo asset would have to widen affected_paths to include it. + val r = WriteScopeRule().assess( + input( + mapOf("source" to "assets/logo.png", "dest" to "public/logo.png"), + setOf(ToolCapability.FILE_WRITE), + FakeProbe(), + paramRoles = mapOf("source" to ParamRole.SOURCE_PATH, "dest" to ParamRole.PATH), + tool = "file_copy", + activeTask = ActiveTask("42", listOf("public/**")), + ), + ) + assertEquals(RiskAction.PROCEED, r.disposition) + assertEquals(listOf("public/logo.png"), r.observations.map { it.facts["path"] }) + } + + @Test + fun `a content-from-disk call is exempt from read-before-write even when the dest exists`() { + // Overwriting an existing binary via a copy must not demand a file_read of it first — that + // read is impossible to satisfy usefully and is exactly the round-trip file_copy removes. + val dest = Path.of("/work/project/public/logo.png") + val r = ReadBeforeWriteRule().assess( + input( + mapOf("source" to "assets/logo.png", "dest" to "public/logo.png"), + setOf(ToolCapability.FILE_WRITE), + FakeProbe(existing = setOf(dest)), + paramRoles = mapOf("source" to ParamRole.SOURCE_PATH, "dest" to ParamRole.PATH), + tool = "file_copy", + ), + ) + assertEquals(RiskAction.PROCEED, r.disposition) + assertTrue(r.issues.isEmpty()) + } + + @Test + fun `a model-authored write still requires a prior read`() { + val target = "/work/project/src/A.kt" + val r = ReadBeforeWriteRule().assess( + input( + mapOf("path" to target), + setOf(ToolCapability.FILE_WRITE), + FakeProbe(existing = setOf(Path.of(target))), + paramRoles = mapOf("path" to ParamRole.PATH), + tool = "file_write", + ), + ) + assertEquals(RiskAction.BLOCK, r.disposition) + assertEquals("READ_BEFORE_WRITE", r.issues.single().code) + } +} diff --git a/core/tools/AGENTS.md b/core/tools/AGENTS.md index 4ef7994c..23863af3 100644 --- a/core/tools/AGENTS.md +++ b/core/tools/AGENTS.md @@ -20,7 +20,8 @@ CORREX kernel team. - `FileMutationRecord` — records file-affecting side effects. - `FileAffectingTool` — extended `Tool` interface for tools that write files; must declare `affectedPaths`. - `OutputCompressionSpec` / `ToolOutputCompressor` / `DeclarativeCompressor` — compress large tool outputs to fit token budgets (Hard Invariant #6: compressed output is informational; original events are preserved). -- `ParamRole` — annotates tool parameter semantic roles (input path, output path, etc.). +- `ParamRole` — annotates tool parameter semantic roles: `PATH` (the path a call acts on / mutates), `SOURCE_PATH` (a path it only reads from while acting on another target, e.g. `file_copy`), `EXEC_COMMAND`, `NETWORK_TARGET`. Plane-2 gates dispatch on these, never on tool names. +- `ToolPath` — the ONE canonical path normalization rule: expands a leading `~`/`~/` to the user home, keeps absolute paths, anchors relative paths on the session's working dir (never the JVM cwd). Every filesystem tool and every plane-2 path rule must resolve through it so policy checks and execution act on the same path. `~other/…` is deliberately NOT expanded. - `ValidationResult` — result from tool-level parameter validation (pre-execution check). - Hard Invariant #5: all tool side effects are captured in events. Silent execution is not allowed. diff --git a/core/tools/src/main/kotlin/com/correx/core/tools/contract/ParamRole.kt b/core/tools/src/main/kotlin/com/correx/core/tools/contract/ParamRole.kt index ca997487..a0592fac 100644 --- a/core/tools/src/main/kotlin/com/correx/core/tools/contract/ParamRole.kt +++ b/core/tools/src/main/kotlin/com/correx/core/tools/contract/ParamRole.kt @@ -1,3 +1,12 @@ package com.correx.core.tools.contract -enum class ParamRole { PATH, EXEC_COMMAND, NETWORK_TARGET } +/** + * The semantic role of a declared tool parameter, so plane-2 rules can judge a call by what each + * argument DOES rather than by the tool's name. + * + * [PATH] is the path a call acts ON (its write/read target). [SOURCE_PATH] is a path a call reads + * FROM while acting on some other target — e.g. `file_copy(source, dest)`. Containment and + * privileged-location gates cover both; write-target gates (read-before-write, stale-write, write + * scope, write manifest) cover only [PATH], because a source is not being mutated. + */ +enum class ParamRole { PATH, SOURCE_PATH, EXEC_COMMAND, NETWORK_TARGET } diff --git a/core/tools/src/main/kotlin/com/correx/core/tools/contract/ToolPath.kt b/core/tools/src/main/kotlin/com/correx/core/tools/contract/ToolPath.kt new file mode 100644 index 00000000..5bee7869 --- /dev/null +++ b/core/tools/src/main/kotlin/com/correx/core/tools/contract/ToolPath.kt @@ -0,0 +1,49 @@ +package com.correx.core.tools.contract + +import java.nio.file.Path +import java.nio.file.Paths + +/** + * The ONE canonical rule for turning a model-supplied path string into the absolute path a tool + * will actually operate on. Every filesystem tool and every plane-2 path rule must resolve through + * here, so the policy/existence check and the execution act on the same path. + * + * The bug this exists to prevent: `~/.gradle/init.d/offline.gradle` used to be treated as a + * RELATIVE path (it is not absolute per `Path.isAbsolute`), resolved to + * `/~/.gradle/init.d/offline.gradle`, reported as "does not exist" — and, worse, as + * *inside* the workspace, so the out-of-workspace approval prompt never fired. The agent was then + * told a real file it had correctly identified was a hallucination. + * + * Rules, in order: + * 1. A leading `~` or `~/` expands to the current user's home. `~other/…` is NOT expanded — only + * the shell knows other users' homes, and guessing one would widen the jail on a lookalike path. + * 2. An absolute path is normalized and used as-is. + * 3. A relative path resolves against [base] (a session's workspace/working dir), never the JVM + * process cwd; with no [base] it falls back to the process cwd. + * + * Symlink resolution and containment stay where they were (`PathJail` / the plane-2 world probe): + * this object only decides WHICH path is meant, not whether it is allowed. + */ +object ToolPath { + + /** Expands a leading `~`/`~/` in [raw] to [home]. Any other string is returned unchanged. */ + fun expandHome(raw: String, home: String? = System.getProperty("user.home")): String = when { + home.isNullOrEmpty() -> raw + raw == "~" -> home + raw.startsWith("~/") -> home + raw.substring(1) + else -> raw + } + + /** + * Resolves [raw] to the absolute, normalized path the tool will operate on: home-expanded, + * then anchored on [base] when relative. + */ + fun resolve(raw: String, base: Path?, home: String? = System.getProperty("user.home")): Path { + val expanded = Paths.get(expandHome(raw, home)) + return when { + expanded.isAbsolute -> expanded.normalize() + base != null -> base.resolve(expanded).normalize() + else -> expanded.toAbsolutePath().normalize() + } + } +} diff --git a/core/tools/src/test/kotlin/com/correx/core/tools/contract/ToolPathTest.kt b/core/tools/src/test/kotlin/com/correx/core/tools/contract/ToolPathTest.kt new file mode 100644 index 00000000..ecfc0f06 --- /dev/null +++ b/core/tools/src/test/kotlin/com/correx/core/tools/contract/ToolPathTest.kt @@ -0,0 +1,71 @@ +package com.correx.core.tools.contract + +import java.nio.file.Path +import kotlin.test.Test +import kotlin.test.assertEquals + +class ToolPathTest { + + private val home = "/home/tester" + private val base: Path = Path.of("/work/project") + + @Test + fun `a leading tilde expands to the user home`() { + // The bug this guards: `~/.gradle/init.d/offline.gradle` used to resolve to + // /~/.gradle/... — reported as non-existent AND as inside the workspace, so the + // agent was told a real file it had correctly identified did not exist. + assertEquals( + Path.of("/home/tester/.gradle/init.d/offline.gradle"), + ToolPath.resolve("~/.gradle/init.d/offline.gradle", base, home), + ) + } + + @Test + fun `a bare tilde is the home directory itself`() { + assertEquals(Path.of("/home/tester"), ToolPath.resolve("~", base, home)) + } + + @Test + fun `another users home is not expanded`() { + // Only the shell knows other users' homes; guessing would widen the jail on a lookalike path. + assertEquals(Path.of("/work/project/~other/notes.md"), ToolPath.resolve("~other/notes.md", base, home)) + } + + @Test + fun `an absolute path is normalized and kept`() { + assertEquals(Path.of("/etc/hosts"), ToolPath.resolve("/etc/./hosts", base, home)) + assertEquals(Path.of("/work/a.txt"), ToolPath.resolve("/work/project/../a.txt", base, home)) + } + + @Test + fun `a relative path anchors on the base, not the process cwd`() { + assertEquals(Path.of("/work/project/src/A.kt"), ToolPath.resolve("src/A.kt", base, home)) + assertEquals(Path.of("/work/project/src/A.kt"), ToolPath.resolve("./src/A.kt", base, home)) + } + + @Test + fun `a relative path escaping the base stays escaped for the containment check to see`() { + // Normalization must not silently clamp `..` back inside: the jail decides, not this object. + assertEquals(Path.of("/work/secrets.txt"), ToolPath.resolve("../secrets.txt", base, home)) + } + + @Test + fun `with no base a relative path falls back to the process cwd`() { + assertEquals( + Path.of("").toAbsolutePath().resolve("src/A.kt").normalize(), + ToolPath.resolve("src/A.kt", null, home), + ) + } + + @Test + fun `an unknown home leaves the tilde untouched`() { + assertEquals(Path.of("/work/project/~/x"), ToolPath.resolve("~/x", base, home = null)) + } + + @Test + fun `expandHome only rewrites a leading tilde`() { + assertEquals("/home/tester/x", ToolPath.expandHome("~/x", home)) + assertEquals("src/~/x", ToolPath.expandHome("src/~/x", home)) + assertEquals("/a/b", ToolPath.expandHome("/a/b", home)) + } +} diff --git a/infrastructure/tools/AGENTS.md b/infrastructure/tools/AGENTS.md index 7ca4cb53..58f3fcc7 100644 --- a/infrastructure/tools/AGENTS.md +++ b/infrastructure/tools/AGENTS.md @@ -16,6 +16,8 @@ Adapter for `core:tools`. Depends on `core:tools`, `core:events`, `core:approval - Web search and web fetch results are environment observations; they must be recorded as events by callers to preserve replay determinism (invariant #9). - `ToolConfig` is the only configuration surface; pass via `InfrastructureModule.createToolExecutor()`. - `buildTools()` extension on `ToolConfig` assembles the full tool list; add new tools there, not in the registry directly. +- `file_copy` copies one existing file to another path (`{source, dest}`) so static/binary assets never pass through the model's token stream. Same jail, anchor and `fileWrite.enabled` toggle as `file_write`; `dest` is the mutated target (`ParamRole.PATH`, the only affected path), `source` is read-only (`ParamRole.SOURCE_PATH`) and may sit under an operator-granted out-of-workspace path exactly as a `file_read` may. One regular file per call: no recursion, no globs. +- Every path parameter resolves through `ToolPath.resolve` (`core:tools`), which expands a leading `~` and anchors relatives on the bound workspace's working dir. Do not re-implement path resolution in a tool. - Filesystem mutation is split by intent: `file_write` only writes (`{path, content}`), `file_edit` edits, and `file_delete` only deletes (`{path}`) — deletion is a separately-named capability so a model can never delete by getting a write-mode parameter wrong. `file_delete` shares `file_write`'s path jail and `fileWrite.enabled` toggle and carries `ToolCapability.FILE_WRITE`. - `list_dir` is shallow by default, but collapses a non-symlink single-child directory chain (bounded depth) to the first branch point and explains that expansion in its output; recursive listings retain normal tree traversal. diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileCopyTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileCopyTool.kt new file mode 100644 index 00000000..136256a6 --- /dev/null +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileCopyTool.kt @@ -0,0 +1,205 @@ +package com.correx.infrastructure.tools.filesystem + +import com.correx.core.approvals.Tier +import com.correx.core.events.events.ToolRequest +import com.correx.core.events.types.ToolInvocationId +import com.correx.core.tools.contract.FileAffectingTool +import com.correx.core.tools.contract.ParamRole +import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolCapability +import com.correx.core.tools.contract.ToolExecutor +import com.correx.core.tools.contract.ToolPath +import com.correx.core.tools.contract.ToolResult +import com.correx.core.tools.contract.ValidationResult +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext +import kotlinx.serialization.json.JsonObject +import kotlinx.serialization.json.JsonPrimitive +import kotlinx.serialization.json.buildJsonArray +import kotlinx.serialization.json.buildJsonObject +import kotlinx.serialization.json.put +import kotlinx.serialization.json.putJsonObject +import java.io.IOException +import java.nio.file.Files +import java.nio.file.InvalidPathException +import java.nio.file.Path +import java.nio.file.Paths +import java.nio.file.StandardCopyOption + +/** + * Copies one file to another path (#713). Without it, moving an existing asset — a static file, an + * image, anything binary — has to go `file_read` → model context → `file_write`, which burns tool + * rounds, floods the context window and corrupts any byte the tokenizer cannot round-trip. A copy + * never puts the bytes in front of the model. + * + * Carries [ToolCapability.FILE_WRITE] (it mutates the filesystem) and shares [FileWriteTool]'s jail + * and toggle. `dest` is the mutated target ([ParamRole.PATH]); `source` is only read + * ([ParamRole.SOURCE_PATH]), so the plane-2 write-target gates (read-before-write, stale-write, + * write scope, write manifest) judge `dest` alone while containment judges both. `source` may + * additionally sit under an operator-approved out-of-workspace path (`grantedPaths`), exactly as a + * `file_read` may; `dest` never can. + * + * ponytail: single regular file per call — no recursive directory copy, no glob. Copying a tree is + * N calls; add recursion only if a real run shows N is the bottleneck. + */ +class FileCopyTool( + allowedPaths: Set = emptySet(), + private val workingDir: Path? = null, +) : Tool, FileAffectingTool, ToolExecutor { + + private val normalizedAllowedPaths: Set = allowedPaths.map { it.normalize().toAbsolutePath() }.toSet() + + override val name: String = "file_copy" + override val description: String = + "Copy the file at 'source' to 'dest' (creates missing parent directories, overwrites an " + + "existing dest). Use this for existing or binary files instead of reading and " + + "re-writing their contents." + override val parametersSchema: JsonObject = buildJsonObject { + put("type", "object") + putJsonObject("properties") { + putJsonObject("source") { + put("type", "string") + put("description", "Relative path of the existing file to copy from") + } + putJsonObject("dest") { + put("type", "string") + put("description", "Relative path to copy to") + } + } + put( + "required", + buildJsonArray { + add(JsonPrimitive("source")) + add(JsonPrimitive("dest")) + }, + ) + } + override val tier: Tier = Tier.T2 + override val requiredCapabilities: Set = setOf(ToolCapability.FILE_WRITE) + override val paramRoles: Map = + mapOf("source" to ParamRole.SOURCE_PATH, "dest" to ParamRole.PATH) + + /** Only `dest` is mutated, so only `dest` gets pre/post CAS images and undo coverage. */ + override fun affectedPaths(request: ToolRequest): Set { + val dest = request.parameters["dest"] as? String ?: return emptySet() + return setOf(ToolPath.resolve(dest, workingDir)) + } + + override fun validateRequest(request: ToolRequest): ValidationResult { + val source = request.parameters["source"] as? String + ?: return ValidationResult.Invalid( + "Missing 'source' parameter (string). Call file_copy with " + + """{"source": "", "dest": ""}.""", + ) + val dest = request.parameters["dest"] as? String + ?: return ValidationResult.Invalid( + """Missing 'dest' parameter (string). Call file_copy with {"source": "$source", "dest": ""}.""", + ) + if (normalizedAllowedPaths.isEmpty()) return ValidationResult.Invalid("No paths are allowed.") + + return runCatching { + // The source is a read, so it may also be an operator-approved out-of-workspace path + // (OutsidePathAccessGrantedEvent), mirroring file_read. The dest is a write: jail only. + val readRoots = normalizedAllowedPaths + request.grantedPaths.map { Paths.get(it) } + when { + !PathJail.isContained(ToolPath.resolve(source, workingDir), readRoots) -> + ValidationResult.Invalid("Path '$source' is not in the allowed list.") + + !PathJail.isContained(ToolPath.resolve(dest, workingDir), normalizedAllowedPaths) -> + ValidationResult.Invalid("Path '$dest' is not in the allowed list.") + + else -> ValidationResult.Valid + } + }.getOrElse { e -> mapExceptionToValidationResult(e) } + } + + private fun mapExceptionToValidationResult(e: Throwable): ValidationResult = + when (e) { + is InvalidPathException -> ValidationResult.Invalid("Invalid path format: ${e.message}") + is IOException -> ValidationResult.Invalid("IO error: ${e.message}") + is SecurityException -> ValidationResult.Invalid("Security error: ${e.message}") + else -> ValidationResult.Invalid(e.message ?: "Unknown error occurred") + } + + override suspend fun execute(request: ToolRequest): ToolResult = withContext(Dispatchers.IO) { + val validation = validateRequest(request) + if (validation is ValidationResult.Invalid) { + return@withContext ToolResult.Failure( + invocationId = request.invocationId, + reason = validation.reason, + recoverable = false, + ) + } + val sourceString = request.parameters["source"] as String + val destString = request.parameters["dest"] as String + val source = ToolPath.resolve(sourceString, workingDir) + val dest = ToolPath.resolve(destString, workingDir) + + // Every rejection below is recoverable: each one is a mistake the agent can correct on the + // next tool round (fix the path, name a file inside the directory) rather than a dead stage. + when { + !Files.exists(source) -> return@withContext ToolResult.Failure( + invocationId = request.invocationId, + reason = "Source file not found: $sourceString", + recoverable = true, + ) + + Files.isDirectory(source) -> return@withContext ToolResult.Failure( + invocationId = request.invocationId, + reason = "Source '$sourceString' is a directory — file_copy copies one file per call.", + recoverable = true, + ) + + Files.isDirectory(dest) -> return@withContext ToolResult.Failure( + invocationId = request.invocationId, + reason = "Dest '$destString' is a directory — name the file inside it " + + "(e.g. '$destString/${source.fileName}').", + recoverable = true, + ) + + source == dest -> return@withContext ToolResult.Failure( + invocationId = request.invocationId, + reason = "Source and dest resolve to the same file ($destString) — nothing to copy.", + recoverable = true, + ) + } + + runCatching { + dest.parent?.let { Files.createDirectories(it) } + Files.copy(source, dest, StandardCopyOption.REPLACE_EXISTING) + ToolResult.Success( + invocationId = request.invocationId, + output = "Copied $sourceString to $destString (${Files.size(dest)} bytes)", + metadata = mapOf("source" to sourceString, "dest" to destString), + ) + }.getOrElse { e -> handleExecutionException(e, request.invocationId, destString) } + } + + private fun handleExecutionException( + e: Throwable, + invocationId: ToolInvocationId, + destString: String, + ): ToolResult = when (e) { + is CancellationException -> throw e + // Recoverable for the same reason as file_write's IO errors: the agent can correct the + // path and retry inside the stage's tool loop. + is IOException -> ToolResult.Failure( + invocationId = invocationId, + reason = "IO error copying to '$destString': ${e.message ?: e.javaClass.simpleName}", + recoverable = true, + ) + + is SecurityException -> ToolResult.Failure( + invocationId = invocationId, + reason = "Access denied: $destString, ${e.message}", + recoverable = false, + ) + + else -> ToolResult.Failure( + invocationId = invocationId, + reason = e.message ?: "Unknown error occurred", + recoverable = false, + ) + } +} diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileDeleteTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileDeleteTool.kt index 972fe80c..083bef49 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileDeleteTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileDeleteTool.kt @@ -6,6 +6,7 @@ import com.correx.core.events.types.ToolInvocationId import com.correx.core.tools.contract.FileAffectingTool import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolPath import com.correx.core.tools.contract.ToolCapability import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -23,7 +24,6 @@ import java.io.IOException import java.nio.file.Files import java.nio.file.InvalidPathException import java.nio.file.Path -import java.nio.file.Paths /** * Deletes a file. Split out of [FileWriteTool] so deletion is an explicitly-named capability a model @@ -59,12 +59,10 @@ class FileDeleteTool( } private fun resolvePath(pathString: String): Path { - val raw = Paths.get(pathString) - return when { - raw.isAbsolute -> raw.normalize() - workingDir != null -> workingDir.resolve(raw).normalize() - else -> raw.toAbsolutePath().normalize() - } + // ToolPath is the ONE normalization rule (expands a leading `~`, anchors relatives on the + // bound workspace's working dir, never the JVM cwd) so the jail check, the existence check + // and the operation itself all act on the same path. + return ToolPath.resolve(pathString, workingDir) } override fun validateRequest(request: ToolRequest): ValidationResult { diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt index 9d361449..1c016ea7 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileEditTool.kt @@ -6,6 +6,7 @@ import com.correx.core.events.types.ToolInvocationId import com.correx.core.tools.contract.FileAffectingTool import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolPath import com.correx.core.tools.contract.ToolCapability import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -33,12 +34,10 @@ class FileEditTool( private val normalizedAllowedPaths: Set = allowedPaths.map { it.normalize().toAbsolutePath() }.toSet() private fun resolvePath(pathString: String): Path { - val raw = Paths.get(pathString) - return when { - raw.isAbsolute -> raw.normalize() - workingDir != null -> workingDir.resolve(raw).normalize() - else -> raw.toAbsolutePath().normalize() - } + // ToolPath is the ONE normalization rule (expands a leading `~`, anchors relatives on the + // bound workspace's working dir, never the JVM cwd) so the jail check, the existence check + // and the operation itself all act on the same path. + return ToolPath.resolve(pathString, workingDir) } override val name: String = "file_edit" diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt index 9c5c6277..139a6e75 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileReadTool.kt @@ -9,6 +9,7 @@ import com.correx.core.tools.compression.OutputCompressionSpec import com.correx.core.tools.compression.ToolOutputCompressor import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolPath import com.correx.core.tools.contract.ToolCapability import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -38,12 +39,10 @@ class FileReadTool( // process cwd. Mirrors FileWriteTool/FileEditTool so all three jail against the // same anchor and agree with the Plane-2 tool-call-intent containment check. private fun resolvePath(pathString: String): Path { - val raw = Paths.get(pathString) - return when { - raw.isAbsolute -> raw.normalize() - workingDir != null -> workingDir.resolve(raw).normalize() - else -> raw.toAbsolutePath().normalize() - } + // ToolPath is the ONE normalization rule (expands a leading `~`, anchors relatives on the + // bound workspace's working dir, never the JVM cwd) so the jail check, the existence check + // and the operation itself all act on the same path. + return ToolPath.resolve(pathString, workingDir) } override val name: String = "file_read" diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt index db8b17f9..193eff8d 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/FileWriteTool.kt @@ -6,6 +6,7 @@ import com.correx.core.events.types.ToolInvocationId import com.correx.core.tools.contract.FileAffectingTool import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolPath import com.correx.core.tools.contract.ToolCapability import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -23,7 +24,6 @@ import java.io.IOException import java.nio.file.Files import java.nio.file.InvalidPathException import java.nio.file.Path -import java.nio.file.Paths /** * Writes content to a file. Write-only by design: deleting is the separate, explicitly-named @@ -73,12 +73,10 @@ class FileWriteTool( } private fun resolvePath(pathString: String): Path { - val raw = Paths.get(pathString) - return when { - raw.isAbsolute -> raw.normalize() - workingDir != null -> workingDir.resolve(raw).normalize() - else -> raw.toAbsolutePath().normalize() - } + // ToolPath is the ONE normalization rule (expands a leading `~`, anchors relatives on the + // bound workspace's working dir, never the JVM cwd) so the jail check, the existence check + // and the operation itself all act on the same path. + return ToolPath.resolve(pathString, workingDir) } override fun validateRequest(request: ToolRequest): ValidationResult { 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 2589fcf8..88cdd58b 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 @@ -4,6 +4,7 @@ import com.correx.core.approvals.Tier import com.correx.core.events.events.ToolRequest import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolPath import com.correx.core.tools.contract.ToolCapability import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -43,12 +44,10 @@ class ListDirTool( ) : Tool, ToolExecutor { private fun resolvePath(pathString: String): Path { - val raw = Paths.get(pathString) - return when { - raw.isAbsolute -> raw.normalize() - workingDir != null -> workingDir.resolve(raw).normalize() - else -> raw.toAbsolutePath().normalize() - } + // ToolPath is the ONE normalization rule (expands a leading `~`, anchors relatives on the + // bound workspace's working dir, never the JVM cwd) so the jail check, the existence check + // and the operation itself all act on the same path. + return ToolPath.resolve(pathString, workingDir) } override val name: String = "list_dir" diff --git a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/WorkspaceSearchTools.kt b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/WorkspaceSearchTools.kt index 03b3f70a..b5c26daa 100644 --- a/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/WorkspaceSearchTools.kt +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/WorkspaceSearchTools.kt @@ -4,6 +4,7 @@ import com.correx.core.approvals.Tier import com.correx.core.events.events.ToolRequest import com.correx.core.tools.contract.ParamRole import com.correx.core.tools.contract.Tool +import com.correx.core.tools.contract.ToolPath import com.correx.core.tools.contract.ToolCapability import com.correx.core.tools.contract.ToolExecutor import com.correx.core.tools.contract.ToolResult @@ -224,12 +225,7 @@ class GrepTool( private fun resolveSearchPath(request: ToolRequest, workingDir: Path?): Path { val pathString = (request.parameters["path"] as? String)?.takeIf { it.isNotBlank() } ?: "." - val raw = Paths.get(pathString) - return when { - raw.isAbsolute -> raw.normalize() - workingDir != null -> workingDir.resolve(raw).normalize() - else -> raw.toAbsolutePath().normalize() - } + return ToolPath.resolve(pathString, workingDir) } private fun validateSearchPath(request: ToolRequest, allowedPaths: Set, workingDir: Path?): ValidationResult = diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileCopyToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileCopyToolTest.kt new file mode 100644 index 00000000..97d7ff5c --- /dev/null +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileCopyToolTest.kt @@ -0,0 +1,162 @@ +package com.correx.infrastructure.tools.filesystem + +import com.correx.core.events.events.ToolRequest +import com.correx.core.events.types.SessionId +import com.correx.core.events.types.StageId +import com.correx.core.events.types.ToolInvocationId +import com.correx.core.tools.contract.ToolResult +import com.correx.core.tools.contract.ValidationResult +import kotlinx.coroutines.runBlocking +import org.junit.jupiter.api.Assertions.assertArrayEquals +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import java.nio.file.Files +import java.nio.file.Path +import java.util.UUID + +class FileCopyToolTest { + + private val invocationId = ToolInvocationId(UUID.randomUUID().toString()) + + private fun request(parameters: Map, grantedPaths: Set = emptySet()): ToolRequest = + ToolRequest( + invocationId = invocationId, + sessionId = SessionId(UUID.randomUUID().toString()), + stageId = StageId(UUID.randomUUID().toString()), + toolName = "file_copy", + parameters = parameters, + grantedPaths = grantedPaths, + ) + + private fun workspace(): Path = Files.createTempDirectory("file_copy_test") + + @Test + fun `copies bytes without routing them through the model`(): Unit = runBlocking { + val dir = workspace() + val bytes = byteArrayOf(0, 1, 2, -1, -128, 127) + Files.write(dir.resolve("logo.png"), bytes) + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + + val result = tool.execute(request(mapOf("source" to "logo.png", "dest" to "public/assets/logo.png"))) + + assertTrue(result is ToolResult.Success, (result as? ToolResult.Failure)?.reason) + // Parent directories are created, and every byte survives — the point of the tool. + assertArrayEquals(bytes, Files.readAllBytes(dir.resolve("public/assets/logo.png"))) + } + + @Test + fun `only dest is reported as an affected path`(): Unit = runBlocking { + val dir = workspace() + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + assertEquals( + setOf(dir.resolve("b.txt")), + tool.affectedPaths(request(mapOf("source" to "a.txt", "dest" to "b.txt"))), + ) + } + + @Test + fun `overwrites an existing dest`(): Unit = runBlocking { + val dir = workspace() + Files.writeString(dir.resolve("a.txt"), "new") + Files.writeString(dir.resolve("b.txt"), "old") + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + + assertTrue(tool.execute(request(mapOf("source" to "a.txt", "dest" to "b.txt"))) is ToolResult.Success) + assertEquals("new", Files.readString(dir.resolve("b.txt"))) + } + + @Test + fun `a missing source is a recoverable failure`(): Unit = runBlocking { + val dir = workspace() + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + val result = tool.execute(request(mapOf("source" to "ghost.txt", "dest" to "b.txt"))) + assertTrue(result is ToolResult.Failure) + assertTrue((result as ToolResult.Failure).recoverable) + assertTrue(result.reason.contains("Source file not found")) + } + + @Test + fun `a directory source or dest is rejected with the corrective move`(): Unit = runBlocking { + val dir = workspace() + Files.createDirectories(dir.resolve("assets")) + Files.writeString(dir.resolve("a.txt"), "x") + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + + val fromDir = tool.execute(request(mapOf("source" to "assets", "dest" to "b.txt"))) + assertTrue((fromDir as ToolResult.Failure).reason.contains("one file per call")) + + val toDir = tool.execute(request(mapOf("source" to "a.txt", "dest" to "assets"))) + assertTrue((toDir as ToolResult.Failure).reason.contains("assets/a.txt")) + } + + @Test + fun `copying a file onto itself is rejected`(): Unit = runBlocking { + val dir = workspace() + Files.writeString(dir.resolve("a.txt"), "x") + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + val result = tool.execute(request(mapOf("source" to "a.txt", "dest" to "./a.txt"))) + assertTrue((result as ToolResult.Failure).reason.contains("nothing to copy")) + } + + @Test + fun `both source and dest are jailed`(): Unit = runBlocking { + val dir = workspace() + val outside = Files.createTempDirectory("file_copy_outside") + Files.writeString(outside.resolve("other.txt"), "s") + Files.writeString(dir.resolve("a.txt"), "x") + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + + val readEscape = tool.validateRequest( + request(mapOf("source" to outside.resolve("other.txt").toString(), "dest" to "a.txt")), + ) + assertTrue(readEscape is ValidationResult.Invalid) + + val writeEscape = tool.validateRequest( + request(mapOf("source" to "a.txt", "dest" to outside.resolve("copied.txt").toString())), + ) + assertTrue(writeEscape is ValidationResult.Invalid) + } + + @Test + fun `an operator-granted path widens the source jail but never the dest`(): Unit = runBlocking { + val dir = workspace() + val granted = Files.createTempDirectory("file_copy_granted") + Files.writeString(granted.resolve("asset.bin"), "a") + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + val grants = setOf(granted.toString()) + + // Mirrors file_read: an approved out-of-workspace path may be READ from… + assertEquals( + ValidationResult.Valid, + tool.validateRequest( + request(mapOf("source" to granted.resolve("asset.bin").toString(), "dest" to "asset.bin"), grants), + ), + ) + // …but a write target never escapes the workspace, approved or not. + assertTrue( + tool.validateRequest( + request(mapOf("source" to "asset.bin", "dest" to granted.resolve("out.bin").toString()), grants), + ) is ValidationResult.Invalid, + ) + } + + @Test + fun `a tilde path resolves to the user home instead of a phantom workspace path`(): Unit = runBlocking { + val home = Path.of(System.getProperty("user.home")) + val dir = workspace() + val tool = FileCopyTool(allowedPaths = setOf(dir, home), workingDir = dir) + // The canonical normalization applies to every path param, not just file_read's. + assertEquals(setOf(home.resolve("x.bin")), tool.affectedPaths(request(mapOf("dest" to "~/x.bin")))) + } + + @Test + fun `missing parameters name the exact call shape`(): Unit = runBlocking { + val dir = workspace() + val tool = FileCopyTool(allowedPaths = setOf(dir), workingDir = dir) + val noSource = tool.validateRequest(request(mapOf("dest" to "b.txt"))) + assertTrue((noSource as ValidationResult.Invalid).reason.contains("Missing 'source'")) + val noDest = tool.validateRequest(request(mapOf("source" to "a.txt"))) + assertTrue((noDest as ValidationResult.Invalid).reason.contains("Missing 'dest'")) + } +} diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt index 1110784e..fed83fe1 100644 --- a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/FileReadToolTest.kt @@ -200,4 +200,22 @@ class FileReadToolTest { val compressed = tool.outputCompressor.compress(raw, ToolOutputContext(exitCode = 0)) assertEquals("fun main() {\nprintln()\n}", compressed) } + + @Test + fun `a home-relative path is read, not reported as missing`(): Unit = runBlocking { + // The 2026-08 harness bug: `~/…` was treated as relative, resolved under the workspace, and + // reported as "File not found" — telling the agent a real file it had correctly identified + // did not exist. Every path param normalizes through ToolPath now. + val home = java.nio.file.Path.of(System.getProperty("user.home")) + val marker = Files.createTempFile(home, "correx_tilde_read", ".txt") + try { + Files.writeString(marker, "real content") + val tool = FileReadTool(allowedPaths = setOf(home), workingDir = Files.createTempDirectory("ws")) + val result = tool.execute(createRequest("~/${marker.fileName}")) + assertTrue(result is ToolResult.Success, (result as? ToolResult.Failure)?.reason) + assertEquals("real content", (result as ToolResult.Success).output) + } finally { + Files.deleteIfExists(marker) + } + } } diff --git a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/ToolConfig.kt b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/ToolConfig.kt index d1a53945..8b2100b0 100644 --- a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/ToolConfig.kt +++ b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/ToolConfig.kt @@ -1,6 +1,7 @@ package com.correx.infrastructure.tools import com.correx.core.tools.contract.Tool +import com.correx.infrastructure.tools.filesystem.FileCopyTool import com.correx.infrastructure.tools.filesystem.FileDeleteTool import com.correx.infrastructure.tools.filesystem.FileEditTool import com.correx.infrastructure.tools.filesystem.FileReadTool @@ -104,6 +105,14 @@ fun ToolConfig.buildTools(): List = buildList { workingDir = fileWrite.workingDir, ), ) + // file_copy moves existing/binary files without routing their bytes through the model's + // context (#713); same jail, anchor and toggle as the writer. + add( + FileCopyTool( + allowedPaths = fileWrite.allowedPaths, + workingDir = fileWrite.workingDir, + ), + ) } if (fileEdit.enabled) { add(