diff --git a/core/config/src/main/kotlin/com/correx/core/config/AgentInstructionsLoader.kt b/core/config/src/main/kotlin/com/correx/core/config/AgentInstructionsLoader.kt index 26acf784..98b8b152 100644 --- a/core/config/src/main/kotlin/com/correx/core/config/AgentInstructionsLoader.kt +++ b/core/config/src/main/kotlin/com/correx/core/config/AgentInstructionsLoader.kt @@ -5,12 +5,17 @@ import java.nio.file.Path import java.nio.file.Paths /** - * Discovers standing agent instructions (`CLAUDE.md`, `AGENTS.md`) at the bound workspace - * root only — no parent walk, no nested lookup. Mirrors [ProjectProfileLoader]: the loaded - * snapshot is bound as an event so replay reads the recorded fact, never the live file. + * Discovers standing agent instructions (`AGENTS.md`) at the bound workspace root only — no + * parent walk, no nested lookup. Mirrors [ProjectProfileLoader]: the loaded snapshot is bound + * as an event so replay reads the recorded fact, never the live file. + * + * CLAUDE.md is deliberately excluded: it targets the outer assistant (agent-dispatch / model- + * selection guidance) and is noise to an in-workflow execution agent — injecting it bloated and + * polluted the stage context. AGENTS.md holds the correx-agent operating contract (the numbered + * how-to-finish-a-stage steps), which is what the agent actually needs. */ object AgentInstructionsLoader { - private val FILE_NAMES = listOf("CLAUDE.md", "AGENTS.md") + private val FILE_NAMES = listOf("AGENTS.md") fun load(workspaceRoot: String): AgentInstructions { val sections = mutableListOf() diff --git a/core/config/src/test/kotlin/com/correx/core/config/AgentInstructionsLoaderTest.kt b/core/config/src/test/kotlin/com/correx/core/config/AgentInstructionsLoaderTest.kt index 5a66e986..4bf47b5f 100644 --- a/core/config/src/test/kotlin/com/correx/core/config/AgentInstructionsLoaderTest.kt +++ b/core/config/src/test/kotlin/com/correx/core/config/AgentInstructionsLoaderTest.kt @@ -11,17 +11,18 @@ import java.nio.file.Path class AgentInstructionsLoaderTest { @Test - fun `both files present yields both sections and sources`(@TempDir root: Path) { + fun `AGENTS is loaded and CLAUDE is ignored`(@TempDir root: Path) { + // CLAUDE.md is deliberately not an agent-instruction source: it targets the outer assistant + // and polluted the stage context. Only AGENTS.md (the agent operating contract) is loaded. Files.writeString(root.resolve("CLAUDE.md"), "Claude rules") Files.writeString(root.resolve("AGENTS.md"), "Agents rules") val loaded = AgentInstructionsLoader.load(root.toString()) - assertEquals(listOf("CLAUDE.md", "AGENTS.md"), loaded.sources) - assertTrue(loaded.content.contains("# CLAUDE.md"), "content: ${loaded.content}") - assertTrue(loaded.content.contains("Claude rules"), "content: ${loaded.content}") + assertEquals(listOf("AGENTS.md"), loaded.sources) assertTrue(loaded.content.contains("# AGENTS.md"), "content: ${loaded.content}") assertTrue(loaded.content.contains("Agents rules"), "content: ${loaded.content}") + assertFalse(loaded.content.contains("Claude rules"), "content: ${loaded.content}") assertFalse(loaded.isEmpty()) } diff --git a/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt b/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt index 8af26c7a..99f2d2e6 100644 --- a/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt +++ b/core/context/src/main/kotlin/com/correx/core/context/builder/DefaultContextPackBuilder.kt @@ -54,10 +54,13 @@ class DefaultContextPackBuilder( const val DOC_PRUNE_RATIO = 0.6 const val DOC_MIN_CHARS = 1200 const val TIER0_TURNS = 3 - // Large static instruction docs (CLAUDE.md project profile, AGENTS.md / DOX framework) - // injected verbatim every turn. Pruned at DOC_PRUNE_RATIO — gentler than freeform, with - // protected spans (code fences, paths, config keys) kept, so directives survive. - val DOC_SOURCE_TYPES = setOf("projectProfile", "agentInstructions") + // Instruction-doc LLMLingua pruning is retired: it fused load-bearing procedural text + // (the numbered stage_complete / emit_artifact steps in AGENTS.md, the curated + // project.toml profile) into unparseable soup, and the agent could no longer tell how to + // finish a stage. The static block is instead kept small by DROPPING CLAUDE.md at the + // loader — not by garbling what remains. Left empty (not deleted) so the branch is a + // no-op guard rather than dead-code churn; freeform conversation pruning is unaffected. + val DOC_SOURCE_TYPES = emptySet() } override suspend fun build( diff --git a/core/context/src/main/kotlin/com/correx/core/context/compression/ContextClassifier.kt b/core/context/src/main/kotlin/com/correx/core/context/compression/ContextClassifier.kt index 9d998037..583a3c82 100644 --- a/core/context/src/main/kotlin/com/correx/core/context/compression/ContextClassifier.kt +++ b/core/context/src/main/kotlin/com/correx/core/context/compression/ContextClassifier.kt @@ -26,6 +26,13 @@ class ContextClassifier { private companion object { val STATIC_SOURCES = setOf("systemPrompt", "toolSchema", "fewShot") - val STRUCTURED_SOURCES = setOf("toolLog", "artifact", "config", "structured", "steeringNote") + // "assistantToolCall" carries the model's own tool-call JSON (id + function + args). It is + // load-bearing structured history: token-pruning it (it was FREEFORM by role=ASSISTANT) + // shredded the JSON to a bare id + protected path, giving the model amnesia about what it + // had already done → it re-issued the same call and looped. Structured = format-compress + // ok, never pruned. + val STRUCTURED_SOURCES = setOf( + "toolLog", "artifact", "config", "structured", "steeringNote", "assistantToolCall", + ) } } diff --git a/core/context/src/test/kotlin/CompressionPipelineStagesTest.kt b/core/context/src/test/kotlin/CompressionPipelineStagesTest.kt index 3aab42b9..857e9a6d 100644 --- a/core/context/src/test/kotlin/CompressionPipelineStagesTest.kt +++ b/core/context/src/test/kotlin/CompressionPipelineStagesTest.kt @@ -138,24 +138,29 @@ class CompressionPipelineStagesTest { } @Test - fun `large static doc entries are pruned at level 3 while base system prompt is untouched`() = kotlinx.coroutines.runBlocking { + fun `static instruction docs are never token-pruned`() = kotlinx.coroutines.runBlocking { + // Instruction-doc pruning is retired: curated/procedural docs (project profile, AGENTS.md + // how-to) must reach the model verbatim — token-pruning fused them into unparseable soup. val pruner = object : com.correx.core.context.compression.TokenPruner { override suspend fun prune(content: String, protectedSpans: List, targetRatio: Double): String = "DOC-PRUNED" } val builder = com.correx.core.context.builder.DefaultContextPackBuilder( com.correx.core.context.compression.DefaultContextCompressor(), CompressionPolicy(3), tokenPruner = pruner, ) - val bigDoc = "# CLAUDE.md\n" + "guidance line ".repeat(120) // > DOC_MIN_CHARS + val bigProfile = "# Project profile\n" + "convention line ".repeat(120) // > old DOC_MIN_CHARS + val bigInstructions = "# AGENTS.md\n" + "5 stage_complete when done. ".repeat(80) val entries = listOf( - entry("proj", ContextLayer.L0, EntryRole.SYSTEM, bigDoc).copy(sourceType = "projectProfile"), + entry("proj", ContextLayer.L0, EntryRole.SYSTEM, bigProfile).copy(sourceType = "projectProfile"), entry("sys", ContextLayer.L0, EntryRole.SYSTEM, "you are an agent").copy(sourceType = "systemPrompt"), + entry("agi", ContextLayer.L0, EntryRole.SYSTEM, bigInstructions).copy(sourceType = "agentInstructions"), ) val flat = builder.build( com.correx.core.events.types.ContextPackId("p"), com.correx.core.events.types.SessionId("s"), com.correx.core.events.types.StageId("st"), entries, com.correx.core.context.model.TokenBudget(limit = 4000), ).layers.values.flatten() - assertEquals("DOC-PRUNED", flat.first { it.sourceType == "projectProfile" }.content) + assertEquals(bigProfile, flat.first { it.sourceType == "projectProfile" }.content) assertEquals("you are an agent", flat.first { it.sourceType == "systemPrompt" }.content) + assertEquals(bigInstructions, flat.first { it.sourceType == "agentInstructions" }.content) Unit } @@ -165,6 +170,8 @@ class CompressionPipelineStagesTest { assertEquals(ContextClass.STATIC, c.classify(entry("systemPrompt", ContextLayer.L0, EntryRole.SYSTEM))) assertEquals(ContextClass.STRUCTURED, c.classify(entry("toolLog", ContextLayer.L2, EntryRole.TOOL))) assertEquals(ContextClass.STRUCTURED, c.classify(entry("steeringNote", ContextLayer.L2, EntryRole.SYSTEM))) + // Assistant tool-call turns are structured history, never token-pruned (amnesia-loop fix). + assertEquals(ContextClass.STRUCTURED, c.classify(entry("assistantToolCall", ContextLayer.L2, EntryRole.ASSISTANT))) assertEquals(ContextClass.FREEFORM, c.classify(entry("chat", ContextLayer.L1, EntryRole.USER))) } } diff --git a/core/sessions/src/main/kotlin/com/correx/core/sessions/DefaultSessionReducer.kt b/core/sessions/src/main/kotlin/com/correx/core/sessions/DefaultSessionReducer.kt index 32ad5e84..d196721e 100644 --- a/core/sessions/src/main/kotlin/com/correx/core/sessions/DefaultSessionReducer.kt +++ b/core/sessions/src/main/kotlin/com/correx/core/sessions/DefaultSessionReducer.kt @@ -8,6 +8,7 @@ import com.correx.core.events.events.StageCompletedEvent import com.correx.core.events.events.StageFailedEvent import com.correx.core.events.events.StoredEvent import com.correx.core.events.events.TransitionExecutedEvent +import com.correx.core.events.events.WorkflowFailedEvent class DefaultSessionReducer : SessionReducer { @@ -22,6 +23,14 @@ class DefaultSessionReducer : SessionReducer { is StageFailedEvent -> SessionStatus.FAILED + // A failed workflow is terminal for the session: no current workflow recovers after + // one fails (freestyle's planning→execution handoff only fires on WorkflowCompleted). + // Without this, an exhausted-retry WorkflowFailed left the session ACTIVE forever, so + // clients/approval loops never saw a terminal state. WorkflowCompleted is deliberately + // NOT mapped here — planning emits its own mid-session, so it isn't session-terminal. + is WorkflowFailedEvent -> + SessionStatus.FAILED + is StageCompletedEvent, is TransitionExecutedEvent -> SessionStatus.ACTIVE diff --git a/core/sessions/src/test/kotlin/com/correx/core/sessions/DefaultSessionReducerTest.kt b/core/sessions/src/test/kotlin/com/correx/core/sessions/DefaultSessionReducerTest.kt index 6d8db80c..c47ecaf0 100644 --- a/core/sessions/src/test/kotlin/com/correx/core/sessions/DefaultSessionReducerTest.kt +++ b/core/sessions/src/test/kotlin/com/correx/core/sessions/DefaultSessionReducerTest.kt @@ -57,6 +57,19 @@ class DefaultSessionReducerTest { assertNull(initialState.boundProfile) } + @Test + fun `WorkflowFailedEvent flips session status to FAILED`() { + val event = stored( + com.correx.core.events.events.WorkflowFailedEvent( + sessionId = sessionId, + stageId = com.correx.core.events.types.StageId("define_styling_tokens"), + reason = "did not produce declared artifacts", + retryExhausted = true, + ), + ) + assertEquals(SessionStatus.FAILED, reducer.reduce(initialState, event).status) + } + @Test fun `unrelated event leaves boundProfile unchanged`() { val state = initialState.copy( diff --git a/examples/workflows/freestyle_planning.toml b/examples/workflows/freestyle_planning.toml index a58457ce..8cc83709 100644 --- a/examples/workflows/freestyle_planning.toml +++ b/examples/workflows/freestyle_planning.toml @@ -10,7 +10,7 @@ start = "analyst" id = "analyst" prompt = "prompts/analyst_freestyle.md" produces = [{ name = "analysis", kind = "analysis" }] -allowed_tools = ["file_read", "ShellTool", "task_search", "task_context", "task_create", "task_decompose"] +allowed_tools = ["file_read", "list_dir", "shell", "task_search", "task_context", "task_create", "task_decompose"] token_budget = 16384 max_retries = 2 diff --git a/examples/workflows/prompts/architect_freestyle.md b/examples/workflows/prompts/architect_freestyle.md index 51790137..3d9acafa 100644 --- a/examples/workflows/prompts/architect_freestyle.md +++ b/examples/workflows/prompts/architect_freestyle.md @@ -1,8 +1,9 @@ # Architect — Freestyle Execution Plan -You are the architect stage in a freestyle workflow. Your only output is a single JSON -`execution_plan` artifact. Do not write prose design documents; emit the plan and nothing -else. +You are the architect stage in a freestyle workflow. Your only output is the +`execution_plan` artifact, produced by calling the **`emit_artifact`** tool with the plan's +fields filled in. Do not write prose design documents and do not write the plan as a plain +message; call `emit_artifact` and nothing else. ## Inputs available to you @@ -27,7 +28,7 @@ Emit a JSON object that validates against the `execution_plan` schema: "produces": "", "kind": "", "needs": [""], - "tools": ["file_read", "file_write", "file_edit", "ShellTool"] + "tools": ["file_read", "file_write", "file_edit", "shell"] } ], "edges": [ @@ -58,10 +59,12 @@ Emit a JSON object that validates against the `execution_plan` schema: - Declare `needs`: every upstream artifact id the stage's prompt references. Every id in `needs` must be `produces`d by a strictly earlier stage. - Include `tools` per stage as it needs them, using only names from this set: - `file_read`, `file_write`, `file_edit`, `ShellTool`, `task_context`, `task_update`, + `file_read`, `file_write`, `file_edit`, `list_dir`, `shell`, `task_context`, `task_update`, `task_search`. Stages that write or edit files take the file set - (`["file_read", "file_write", "file_edit", "ShellTool"]`). Do not invent names beyond - this set. + (`["file_read", "file_write", "file_edit", "list_dir", "shell"]`). `list_dir` gives a + recursive, `.gitignore`-aware tree (skips `node_modules`/`build`/`dist`) — give it to any + stage that explores or scaffolds a project, so it never needs `shell` `ls -R`/`find`. Do not + invent names beyond this set. - **Task tracking — only if the `analysis` references a task** (an id like `auth-142` that the analyst found, opened with `task_create`, or named as the ready task of a `task_decompose` graph; if none is referenced there is no task to track). Thread **only that 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 new file mode 100644 index 00000000..7d8d15e9 --- /dev/null +++ b/infrastructure/tools/filesystem/src/main/kotlin/com/correx/infrastructure/tools/filesystem/ListDirTool.kt @@ -0,0 +1,201 @@ +package com.correx.infrastructure.tools.filesystem + +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.ToolCapability +import com.correx.core.tools.contract.ToolExecutor +import com.correx.core.tools.contract.ToolResult +import com.correx.core.tools.contract.ValidationResult +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.nio.file.FileVisitResult +import java.nio.file.Files +import java.nio.file.InvalidPathException +import java.nio.file.Path +import java.nio.file.Paths +import java.nio.file.SimpleFileVisitor +import java.nio.file.attribute.BasicFileAttributes +import kotlin.io.path.name +import kotlin.io.path.readText + +/** + * Recursive, `.gitignore`-aware directory listing — the affordance weak local models should reach + * for instead of shell `ls -R`, which dumps `node_modules`/`build`/`dist` and drowns a small model + * in noise. Read-only (Tier T1, FILE_READ): it shares [FileReadTool]'s path jail and workspace + * anchor. `.gitignore` is honoured for *enumeration only* — the mutation tools deliberately do NOT + * respect it (you legitimately write to ignored paths like `.env`/`dist`). + */ +class ListDirTool( + private val allowedPaths: Set = emptySet(), + private val workingDir: Path? = null, +) : 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() + } + } + + override val name: String = "list_dir" + override val description: String = + "List a directory tree, skipping anything matched by .gitignore (node_modules, build output, " + + "logs, etc). Prefer this over shell 'ls -R' or 'find' for exploring project structure." + override val parametersSchema: JsonObject = buildJsonObject { + put("type", "object") + putJsonObject("properties") { + putJsonObject("path") { + put("type", "string") + put("description", "Relative directory to list. Omit or '.' for the workspace root.") + } + putJsonObject("recursive") { + put("type", "boolean") + put("description", "Descend into sub-directories (gitignored subtrees are pruned). Default true.") + } + } + put("required", buildJsonArray {}) + } + override val tier: Tier = Tier.T1 + override val requiredCapabilities: Set = setOf(ToolCapability.FILE_READ) + override val paramRoles: Map = mapOf("path" to ParamRole.PATH) + + override fun validateRequest(request: ToolRequest): ValidationResult { + val pathString = (request.parameters["path"] as? String) ?: "." + return runCatching { + val path = resolvePath(pathString) + if (!PathJail.isContained(path, allowedPaths)) + ValidationResult.Invalid("Path '$pathString' is not in the allowed list") + else ValidationResult.Valid + }.getOrElse { + when (it) { + is InvalidPathException -> ValidationResult.Invalid("Invalid path format: ${it.message}") + else -> ValidationResult.Invalid(it.message ?: "Unknown error occurred") + } + } + } + + override suspend fun execute(request: ToolRequest): ToolResult = withContext(Dispatchers.IO) { + (validateRequest(request) as? ValidationResult.Invalid)?.let { + return@withContext ToolResult.Failure(request.invocationId, it.reason, recoverable = false) + } + val pathString = (request.parameters["path"] as? String) ?: "." + val recursive = request.parameters["recursive"]?.toString()?.toBoolean() ?: true + val root = resolvePath(pathString) + when { + !Files.exists(root) -> + ToolResult.Failure(request.invocationId, "Directory not found: $pathString", recoverable = true) + !Files.isDirectory(root) -> + ToolResult.Failure(request.invocationId, "Not a directory: $pathString", recoverable = true) + else -> runCatching { walk(root, recursive, request) }.getOrElse { + ToolResult.Failure(request.invocationId, "Failed to list dir: ${it.message}", recoverable = false) + } + } + } + + private fun walk(root: Path, recursive: Boolean, request: ToolRequest): ToolResult { + val matcher = GitignoreMatcher(workingDir ?: root, root) + val out = ArrayList() + var truncated = false + // Emit one entry; TERMINATE the walk once the cap is hit so a huge tree can't run away. + fun emit(path: Path, isDir: Boolean): FileVisitResult { + val slash = if (isDir) "/" else "" + out += root.relativize(path).toString().replace('\\', '/') + slash + if (out.size >= MAX_ENTRIES) truncated = true + return if (truncated) FileVisitResult.TERMINATE else FileVisitResult.CONTINUE + } + val maxDepth = if (recursive) Int.MAX_VALUE else 1 + Files.walkFileTree(root, emptySet(), maxDepth, object : SimpleFileVisitor() { + override fun preVisitDirectory(dir: Path, attrs: BasicFileAttributes): FileVisitResult { + if (dir != root && (dir.name == ".git" || matcher.ignored(dir, isDir = true))) + return FileVisitResult.SKIP_SUBTREE + matcher.loadFrom(dir) + return if (dir == root) FileVisitResult.CONTINUE else emit(dir, isDir = true) + } + override fun visitFile(file: Path, attrs: BasicFileAttributes): FileVisitResult { + // A directory sitting exactly at maxDepth is delivered here (walkFileTree won't + // descend it), so honour its dir-ness for both the ignore check and the trailing '/'. + val isDir = attrs.isDirectory + return if (matcher.ignored(file, isDir)) FileVisitResult.CONTINUE else emit(file, isDir) + } + override fun visitFileFailed(file: Path, exc: java.io.IOException) = FileVisitResult.CONTINUE + }) + out.sort() + val body = out.joinToString("\n").ifEmpty { "(empty)" } + val note = + if (truncated) "\n… truncated at $MAX_ENTRIES entries; narrow the path or list a sub-directory." else "" + return ToolResult.Success(request.invocationId, output = body + note) + } + + private companion object { + const val MAX_ENTRIES = 400 + } +} + +/** + * Minimal `.gitignore` matcher: enough to prune the common noise (node_modules, dist, build, logs, + * editor dirs). Loads a `.gitignore` per directory as the walk descends, plus the target's ancestors + * once at construction, so nested ignore files scope to their own subtree. + * + * ponytail: no negation (`!`) and `**` is treated as a loose `.*`. If a real repo needs full + * gitignore semantics, back this with `git check-ignore` / a spec-complete matcher. + */ +internal class GitignoreMatcher(anchor: Path, target: Path) { + private data class Rule(val base: Path, val regex: Regex, val dirOnly: Boolean) + + private val rules = ArrayList() + + init { + // Ancestor .gitignore files (target up to and including anchor) apply to the whole listing; + // load them outermost-first so the target's own rules are added last. + generateSequence(target) { if (it == anchor) null else it.parent } + .toList().asReversed().forEach { loadFrom(it) } + } + + fun loadFrom(dir: Path) { + val gi = dir.resolve(".gitignore") + if (!Files.isRegularFile(gi)) return + runCatching { gi.readText() }.getOrNull()?.lineSequence()?.forEach { line -> + val pat = line.trim() + if (pat.isEmpty() || pat.startsWith("#") || pat.startsWith("!")) return@forEach + rules += compile(dir, pat) + } + } + + fun ignored(path: Path, isDir: Boolean): Boolean = rules.any { r -> + (!r.dirOnly || isDir) && path.startsWith(r.base) && + r.regex.matches(r.base.relativize(path).toString().replace('\\', '/')) + } + + private fun compile(base: Path, raw: String): Rule { + var pat = raw + val dirOnly = pat.endsWith("/").also { if (it) pat = pat.dropLast(1) } + val anchored = pat.startsWith("/").also { if (it) pat = pat.drop(1) } + val body = StringBuilder() + var i = 0 + while (i < pat.length) { + val c = pat[i] + when (c) { + '*' -> if (i + 1 < pat.length && pat[i + 1] == '*') { body.append(".*"); i++ } else body.append("[^/]*") + '?' -> body.append("[^/]") + '.', '(', ')', '+', '|', '^', '$', '{', '}', '[', ']', '\\' -> body.append('\\').append(c) + else -> body.append(c) + } + i++ + } + // A slash-less, unanchored pattern matches the entry at any depth (git semantics); otherwise + // it is anchored to the .gitignore's directory. Trailing "(/.*)?" so a matched dir also + // covers everything under it. + val prefix = if (anchored || pat.contains('/')) "" else "(.*/)?" + return Rule(base, Regex("$prefix$body(/.*)?"), dirOnly) + } +} diff --git a/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt new file mode 100644 index 00000000..b74bfe0a --- /dev/null +++ b/infrastructure/tools/filesystem/src/test/kotlin/com/correx/infrastructure/tools/filesystem/ListDirToolTest.kt @@ -0,0 +1,57 @@ +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 kotlinx.coroutines.runBlocking +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import java.nio.file.Files +import java.util.* + +class ListDirToolTest { + + private fun request(path: String? = null, recursive: Boolean? = null) = ToolRequest( + invocationId = ToolInvocationId(UUID.randomUUID().toString()), + sessionId = SessionId(UUID.randomUUID().toString()), + stageId = StageId(UUID.randomUUID().toString()), + toolName = "list_dir", + parameters = buildMap { + path?.let { put("path", it) } + recursive?.let { put("recursive", it) } + }, + ) + + @Test + fun `recursive listing prunes gitignored dirs and files`(): Unit = runBlocking { + val root = Files.createTempDirectory("listdir") + Files.writeString(root.resolve(".gitignore"), "node_modules\ndist\n*.log\n") + Files.createDirectories(root.resolve("node_modules/pkg")).also { Files.writeString(it.resolve("x.js"), "x") } + Files.createDirectories(root.resolve("dist")).also { Files.writeString(it.resolve("bundle.js"), "b") } + Files.createDirectories(root.resolve("src")).also { Files.writeString(it.resolve("main.ts"), "m") } + Files.writeString(root.resolve("debug.log"), "noise") + Files.writeString(root.resolve("package.json"), "{}") + + val tool = ListDirTool(allowedPaths = setOf(root), workingDir = root) + val out = (tool.execute(request()) as ToolResult.Success).output + + assertTrue(out.contains("src/main.ts"), out) + assertTrue(out.contains("package.json"), out) + assertFalse(out.contains("node_modules"), out) // whole subtree pruned, never descended + assertFalse(out.contains("dist"), out) + assertFalse(out.contains("debug.log"), out) + } + + @Test + fun `non-recursive lists only immediate children`(): Unit = runBlocking { + val root = Files.createTempDirectory("listdir") + Files.createDirectories(root.resolve("src/deep")).also { Files.writeString(it.resolve("f.ts"), "f") } + val tool = ListDirTool(allowedPaths = setOf(root), workingDir = root) + val out = (tool.execute(request(recursive = false)) as ToolResult.Success).output + assertTrue(out.contains("src/"), out) + assertFalse(out.contains("deep"), out) + } +} 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 54288f27..1890663e 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 @@ -5,6 +5,7 @@ import com.correx.infrastructure.tools.filesystem.FileDeleteTool import com.correx.infrastructure.tools.filesystem.FileEditTool import com.correx.infrastructure.tools.filesystem.FileReadTool import com.correx.infrastructure.tools.filesystem.FileWriteTool +import com.correx.infrastructure.tools.filesystem.ListDirTool import com.correx.infrastructure.tools.shell.ShellTool import com.correx.infrastructure.tools.web.WebFetchTool import com.correx.infrastructure.tools.web.WebSearchTool @@ -74,6 +75,13 @@ fun ToolConfig.buildTools(): List = buildList { workingDir = fileRead.workingDir, ), ) + // list_dir is read-only enumeration; shares the reader's jail + anchor and the same toggle. + add( + ListDirTool( + allowedPaths = fileRead.allowedPaths, + workingDir = fileRead.workingDir, + ), + ) } if (fileWrite.enabled) { add( diff --git a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt index 7ff09e29..8a3444c4 100644 --- a/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt +++ b/infrastructure/tools/src/main/kotlin/com/correx/infrastructure/tools/shell/ShellTool.kt @@ -41,7 +41,12 @@ class ShellTool( putJsonObject("argv") { put("type", "array") putJsonObject("items") { put("type", "string") } - put("description", "Command and arguments as a list. Example: [\"bash\", \"script.sh\"]") + put( + "description", + "Command and arguments as a list, e.g. [\"npm\", \"install\"]. A shell command " + + "line also works — cd, &&, pipes and redirects run via a shell, e.g. " + + "[\"cd\", \"apps/web-ui\", \"&&\", \"npm\", \"install\"].", + ) } } put("required", buildJsonArray { add(JsonPrimitive("argv")) }) @@ -61,38 +66,87 @@ class ShellTool( ), ) - override fun validateRequest(request: ToolRequest): ValidationResult { - val argv = when (val raw = request.parameters["argv"]) { - is List<*> -> raw.filterIsInstance() - is String -> runCatching { - Json.decodeFromString>(raw) - }.getOrElse { emptyList() } - else -> emptyList() + override fun validateRequest(request: ToolRequest): ValidationResult = + when (val parsed = parseArgv(request.parameters["argv"])) { + is ArgvParse.Bad -> ValidationResult.Invalid(parsed.reason) + is ArgvParse.Ok -> when { + allowedExecutables.isNotEmpty() && parsed.argv[0] !in allowedExecutables -> + ValidationResult.Invalid("Executable '${parsed.argv[0]}' is not in the allowed list.") + else -> ValidationResult.Valid + } } - return when { - argv.isEmpty() -> - ValidationResult.Invalid("Missing or empty 'argv' parameter. Expected List.") - allowedExecutables.isNotEmpty() && argv[0] !in allowedExecutables -> - ValidationResult.Invalid("Executable '${argv[0]}' is not in the allowed list.") - - else -> ValidationResult.Valid - } + private sealed interface ArgvParse { + data class Ok(val argv: List) : ArgvParse + data class Bad(val reason: String) : ArgvParse } + // Weak local models emit malformed argv in two recurring shapes: (1) flags as JSON numbers, + // e.g. ["mkdir", -1, ...] where "-p" should be — AnyMapSerializer decodes these to Long/Double, + // and blindly stringifying them feeds the shell a literal "-1" ("invalid option"); (2) the whole + // array collapsed into one escaped string, e.g. ["npm\",\"-v\","] → argv[0] = npm","-v"," which + // is not a runnable program. Reject both here with an actionable message so the model re-emits a + // clean array on retry, instead of burning a stage on a cryptic exec failure. + private fun parseArgv(raw: Any?): ArgvParse = when (raw) { + is List<*> -> { + val nonString = raw.withIndex().firstOrNull { it.value !is String } + when { + raw.isEmpty() -> ArgvParse.Bad(EMPTY_MSG) + nonString != null -> { + val type = nonString.value?.let { it::class.simpleName } ?: "null" + ArgvParse.Bad( + "argv element at index ${nonString.index} is a $type (`${nonString.value}`), not a string. " + + "Emit argv as a JSON array of strings — quote every token, " + + "e.g. [\"mkdir\", \"-p\", \"dir\"] (not [\"mkdir\", -p]).", + ) + } + else -> checkExecutable(raw.map { it as String }) + } + } + is String -> runCatching { Json.decodeFromString>(raw) } + .fold({ if (it.isEmpty()) ArgvParse.Bad("Empty 'argv'.") else checkExecutable(it) }, { + ArgvParse.Bad("'argv' must be a JSON array of strings, e.g. [\"ls\", \"-la\"].") + }) + else -> ArgvParse.Bad(EMPTY_MSG) + } + + // The model meant a shell command line (not a lone program) when argv[0] is a shell builtin or + // any token is a shell operator — route those through `sh -c` instead of exec-ing directly. + private fun looksLikeShellCommand(argv: List): Boolean = + argv[0] in SHELL_BUILTINS || argv.any { it in SHELL_OPERATORS } + + // argv[0] is the program name; embedded quotes/commas/whitespace mean the array collapsed into + // one string element and there is no real executable to run. + private fun checkExecutable(argv: List): ArgvParse = + if (argv[0].any { it == '"' || it == ',' || it.isWhitespace() }) { + ArgvParse.Bad( + "argv[0] `${argv[0]}` is not a valid executable name — it looks like a collapsed array. " + + "Emit each token as a separate string element, e.g. [\"npm\", \"-v\"].", + ) + } else ArgvParse.Ok(argv) + override suspend fun execute(request: ToolRequest): ToolResult = withContext(Dispatchers.IO) { validateRequest(request).run { takeIf { this is ValidationResult.Valid }?.let { - val argv = when (val raw = request.parameters["argv"]) { - is List<*> -> raw.filterIsInstance() - is String -> runCatching { - Json.decodeFromString>(raw) - }.getOrElse { emptyList() } - else -> emptyList() + val argv = (parseArgv(request.parameters["argv"]) as ArgvParse.Ok).argv + // Weak models reliably emit shell command lines (`cd x && npm i`, pipes, redirects) + // rather than a lone program — that's the near-universal agent expectation. Rather + // than fight it, honour it: when argv looks like a shell command (a builtin at [0] or + // an operator token), run it through `sh -c` so cd/&&/|/> work. A plain program still + // execs directly (preserves exact arg quoting). ponytail: joining tokens for sh -c + // loses quoting of args containing spaces — fine for agent shell use, not a general + // shell API. A still-unrunnable program returns RECOVERABLE (model adapts, not fatal). + val command = if (looksLikeShellCommand(argv)) listOf("sh", "-c", argv.joinToString(" ")) else argv + val process = runCatching { + ProcessBuilder(command).apply { workingDir?.let { directory(it.toFile()) } }.start() + }.getOrElse { + return@let ToolResult.Failure( + invocationId = request.invocationId, + reason = "Could not execute '${argv[0]}': ${it.message}. Pass a runnable program, " + + "or a shell command line (cd/&&/pipes are run via 'sh -c').", + recoverable = true, + ) } - val process = ProcessBuilder(argv).apply { - workingDir?.let { directory(it.toFile()) } - }.start() runCatching { runCmd(request, process) }.getOrElse { @@ -122,6 +176,9 @@ class ShellTool( private companion object { const val HEAD_LINES = 40 const val TAIL_LINES = 40 + const val EMPTY_MSG = "Missing or empty 'argv' parameter. Expected a JSON array of strings." + val SHELL_BUILTINS = setOf("cd", "export", "source", ".", "pushd", "popd", "umask", "ulimit") + val SHELL_OPERATORS = setOf("&&", "||", "|", ">", ">>", "<", ";", "&", "2>", "2>&1") } private suspend fun runCmd(request: ToolRequest, process: Process): ToolResult = withTimeout(timeoutMs) { @@ -134,10 +191,13 @@ class ShellTool( val metadata = if (stderr.isNotBlank()) mapOf("stderr" to stderr) else emptyMap() if (exitCode != 0) { + // A non-zero exit is normal command behaviour (missing path, no grep match, git + // --exit-code, test, diff). Surface it recoverably so the model sees the code + stderr + // and adapts, rather than aborting the stage on the first failed probe. ToolResult.Failure( invocationId = request.invocationId, reason = "Process exited with code $exitCode${if (stderr.isNotBlank()) ": $stderr" else ""}", - recoverable = false, + recoverable = true, ) } else { ToolResult.Success( diff --git a/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt b/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt index 1599b099..05150022 100644 --- a/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt +++ b/infrastructure/tools/src/test/kotlin/com/correx/infrastructure/tools/shell/ShellToolTest.kt @@ -53,7 +53,7 @@ class ShellToolTest { val result = tool.validateRequest(request) assertTrue(result is ValidationResult.Invalid) assertEquals( - "Missing or empty 'argv' parameter. Expected List.", + "Missing or empty 'argv' parameter. Expected a JSON array of strings.", (result as ValidationResult.Invalid).reason, ) } @@ -71,7 +71,7 @@ class ShellToolTest { val result = tool.validateRequest(request) assertTrue(result is ValidationResult.Invalid) assertEquals( - "Missing or empty 'argv' parameter. Expected List.", + "Missing or empty 'argv' parameter. Expected a JSON array of strings.", (result as ValidationResult.Invalid).reason, ) } @@ -99,6 +99,7 @@ class ShellToolTest { val failure = result as ToolResult.Failure assertTrue(failure.reason.contains("exited with code 1")) assertEquals(invocationId, failure.invocationId) + assertTrue(failure.recoverable, "non-zero exit must be recoverable so the model can adapt") } @Test @@ -134,4 +135,49 @@ class ShellToolTest { val compressed = tool.outputCompressor.compress(raw, ToolOutputContext(exitCode = 1)) assertEquals(raw, compressed) } + + private fun rawArgvRequest(argv: List) = ToolRequest( + invocationId = invocationId, sessionId = sessionId, stageId = stageId, + toolName = "shell", parameters = mapOf("argv" to argv), + ) + + @Test + fun `validateRequest rejects non-string argv element with actionable message`() { + // Repro: gemma emitted ["mkdir", -1, "dir"] — AnyMapSerializer decodes -1 to a Long, which + // used to be silently dropped and stringified to "-1" at exec ("invalid option -- '1'"). + val result = ShellTool().validateRequest(rawArgvRequest(listOf("mkdir", -1L, "dir"))) + assertTrue(result is ValidationResult.Invalid) + val reason = (result as ValidationResult.Invalid).reason + assertTrue(reason.contains("index 1"), reason) + assertTrue(reason.contains("string"), reason) + } + + @Test + fun `validateRequest rejects collapsed-array argv0`() { + // Repro: ["npm\",\"-v\","] — the whole array collapsed into argv[0]. + val result = ShellTool().validateRequest(rawArgvRequest(listOf("npm\",\"-v\","))) + assertTrue(result is ValidationResult.Invalid) + assertTrue((result as ValidationResult.Invalid).reason.contains("collapsed array"), result.reason) + } + + @Test + fun `execute runs a shell command line via sh -c`(): Unit = runBlocking { + // Repro: gemma called shell with a builtin/command-line ("cd apps && ..."). Now honoured + // through sh -c instead of the IOException escaping uncaught and killing the workflow. + val result = ShellTool().execute(createRequest(listOf("cd", "/tmp", "&&", "echo", "ok"))) + assertTrue(result is ToolResult.Success, "shell command line should run: $result") + assertTrue((result as ToolResult.Success).output.trim() == "ok", result.output) + } + + @Test + fun `execute returns recoverable Failure when program is genuinely not runnable`(): Unit = runBlocking { + val result = ShellTool().execute(createRequest(listOf("definitely-not-a-real-binary-xyz"))) + assertTrue(result is ToolResult.Failure) + assertTrue((result as ToolResult.Failure).recoverable, "unrunnable program must be recoverable") + } + + @Test + fun `validateRequest accepts a clean string argv`() { + assertEquals(ValidationResult.Valid, ShellTool().validateRequest(createRequest(listOf("mkdir", "-p", "dir")))) + } } diff --git a/sidecars/llmlingua/server.py b/sidecars/llmlingua/server.py index 2a3d2bd6..1b087cd8 100644 --- a/sidecars/llmlingua/server.py +++ b/sidecars/llmlingua/server.py @@ -55,11 +55,18 @@ def prune(req: PruneRequest): text = req.text.strip() if not text: return PruneResponse(compressed=req.text) + compressor = _get_compressor() + # LLMLingua-2 asserts len(force_tokens) <= max_force_token (default 100). Large static docs + # (CLAUDE.md/AGENTS.md) yield hundreds of protected spans, which used to blow the assert and + # 500 -> the doc came back uncompressed. Dedup and keep the longest spans (most load-bearing: + # full paths, hashes, code fences beat bare numbers) up to the cap. + cap = getattr(compressor, "max_force_token", 100) + forced = sorted(set(req.protected), key=len, reverse=True)[:cap] or None # rate is fraction to keep; LLMLingua-2 force_tokens keeps the protected spans verbatim. - result = _get_compressor().compress_prompt( + result = compressor.compress_prompt( text, rate=max(0.1, min(1.0, req.rate)), - force_tokens=req.protected or None, + force_tokens=forced, drop_consecutive=True, ) return PruneResponse(compressed=result["compressed_prompt"])