fix(context,tools): context hygiene + tool ergonomics from freestyle QA
Found while live-QAing freestyle_planning on a 12B local model: - list_dir tool: recursive, .gitignore-aware listing so weak models stop flooding context with `ls -R` over node_modules/build/dist. Wired into the fileRead toggle + advertised to the planner (architect_freestyle). - ContextClassifier: assistantToolCall turns are STRUCTURED, so the token pruner never shreds the model's own tool-call history — that was causing amnesia loops (re-issuing calls it had already made). - Retire instruction-doc LLMLingua pruning (DOC_SOURCE_TYPES emptied): it fused load-bearing procedural text into unparseable soup. The static block stays small by dropping CLAUDE.md at the loader instead. - AgentInstructionsLoader: load only AGENTS.md, not CLAUDE.md — the latter targets the outer assistant and polluted the agent's stage context. - DefaultSessionReducer: WorkflowFailed flips session status to FAILED (was stuck ACTIVE forever, so clients/approval loops never saw a terminal). - ShellTool: run shell command lines (cd/&&/pipes) via `sh -c`; unrunnable program is recoverable instead of an uncaught IOException killing the stage; malformed argv (non-string/collapsed-array) rejected with guidance. - llmlingua sidecar: cap force_tokens to max_force_token (big docs blew the assert and 500'd, so doc pruning silently failed open). Tests added/updated across all of the above.
This commit is contained in:
@@ -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<Tool> = 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(
|
||||
|
||||
+86
-26
@@ -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<String>()
|
||||
is String -> runCatching {
|
||||
Json.decodeFromString<List<String>>(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<String>.")
|
||||
|
||||
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<String>) : 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<List<String>>(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<String>): 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<String>): 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<String>()
|
||||
is String -> runCatching {
|
||||
Json.decodeFromString<List<String>>(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(
|
||||
|
||||
+48
-2
@@ -53,7 +53,7 @@ class ShellToolTest {
|
||||
val result = tool.validateRequest(request)
|
||||
assertTrue(result is ValidationResult.Invalid)
|
||||
assertEquals(
|
||||
"Missing or empty 'argv' parameter. Expected List<String>.",
|
||||
"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<String>.",
|
||||
"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<Any>) = 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"))))
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user