diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/NodeStreamingChatHelper.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/NodeStreamingChatHelper.java index ba5ecd355..e6a9f415e 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/NodeStreamingChatHelper.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/NodeStreamingChatHelper.java @@ -1764,7 +1764,7 @@ static Prompt normalizeToolCallArguments(Prompt prompt) { List calls = am.getToolCalls(); for (int j = 0; j < calls.size(); j++) { AssistantMessage.ToolCall tc = calls.get(j); - String safe = sanitizeToolCallArguments(tc.name(), tc.arguments()); + String safe = sanitizeToolCallArguments(tc.name(), tc.arguments(), false); if (!safe.equals(tc.arguments())) { if (fixedCalls == null) fixedCalls = new ArrayList<>(calls); fixedCalls.set(j, new AssistantMessage.ToolCall(tc.id(), tc.type(), tc.name(), safe)); @@ -2403,13 +2403,14 @@ private List buildFinalToolCalls(List * Some providers (e.g. aliyun-codingplan) reject the entire follow-up * request with HTTP 400 when the assistant message in history carries a @@ -2421,23 +2422,53 @@ private List buildFinalToolCalls(ListThe upstream stream is truncated mid-token, leaving a partial * JSON fragment like {@code "{\"a\":"}. * - * Both cases are normalized to {@code "{}"} so the chat-completions - * round-trip stays valid. Tool execution downstream still re-validates - * arguments and surfaces a per-tool error if the empty payload is wrong - * for that tool. + * + *

The two call sites pass different {@code aggregationPoint} values: + *

    + *
  • Outbound chokepoint ({@code aggregationPoint=false}, + * {@link #normalizeToolCallArguments}): every assistant tool call + * replayed from history is harmonized to an empty JSON object when + * missing/invalid, so the chat-completions round-trip stays valid.
    • + *
  • Stream aggregation ({@code aggregationPoint=true}, + * {@link #buildFinalToolCalls}): the current turn's freshly + * accumulated calls. Blank arguments still become an empty JSON object + * (but now logged — a blank payload was previously invisible), while a + * truncated/invalid fragment is passed through verbatim so the + * tool executor's JSON pre-validation detects the truncation and returns + * the model a "shorten and retry" guidance instead of a generic per-tool + * "missing argument" error. History stays protected because the outbound + * chokepoint re-normalizes the persisted fragment on the next send.
    • + *
*/ - private static String sanitizeToolCallArguments(String toolName, String arguments) { + private static String sanitizeToolCallArguments(String toolName, String arguments, boolean aggregationPoint) { if (arguments == null || arguments.isBlank()) { + if (aggregationPoint) { + log.warn("Tool '{}' tool-call arguments are null/blank right after stream " + + "aggregation; normalizing to an empty JSON object. Either the " + + "model emitted an empty argument payload or every argument chunk " + + "was lost before aggregation.", + toolName); + } return "{}"; } try { TOOL_ARG_JSON_MAPPER.readTree(arguments); return arguments; } catch (Exception e) { - log.warn("Tool '{}' arguments are not valid JSON after stream aggregation " - + "(len={}, head={}); replacing with empty object so the " - + "follow-up chat-completions request stays well-formed. " - + "Parse error: {}", + if (aggregationPoint) { + log.warn("Tool '{}' arguments are not valid JSON after stream aggregation " + + "(len={}, head={}); passing the raw fragment through so the tool " + + "executor's truncation detection can tell the model to shorten and " + + "retry. Parse error: {}", + toolName, + arguments.length(), + arguments.substring(0, Math.min(80, arguments.length())), + e.getMessage()); + return arguments; + } + log.warn("Tool '{}' arguments are not valid JSON (len={}, head={}); replacing with " + + "empty object so the follow-up chat-completions request stays " + + "well-formed. Parse error: {}", toolName, arguments.length(), arguments.substring(0, Math.min(80, arguments.length())), diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java index 9aae7949f..3b6de8cde 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolExecutionExecutor.java @@ -538,7 +538,15 @@ public ToolExecutionResult execute(List toolCalls, } catch (Exception jsonEx) { log.warn("[ToolExecutor] Tool {} arguments invalid/truncated JSON (len={}): {}", toolName, arguments.length(), jsonEx.getMessage()); - String truncationError = normalizeToolExecutionError(jsonEx); + // This guard wraps only readTree(arguments), so ANY exception reaching + // here is a JSON parse failure: the arguments were truncated mid-token + // (Jackson: "Unexpected end-of-input") or otherwise malformed (e.g. + // {"a":} → "Unexpected character"). Return the truncation guidance + // unconditionally instead of pattern-matching the parser message — the + // message phrasing varies with the malformation kind, but the remedy is + // always the same (re-emit the call). normalizeToolExecutionError still + // pattern-matches for genuine tool-execution errors at the other sites. + String truncationError = TRUNCATED_ARGS_GUIDANCE; events.add(GraphEventPublisher.toolComplete(toolCall.id(), toolName, truncationError, false)); allResponses.add(new ToolResponseMessage.ToolResponse( toolCall.id(), toolName, truncationError)); @@ -1174,7 +1182,27 @@ private String classifyBatchPhase(List preparedCalls) { return memoryOnly ? "reading_memory" : "executing_tool"; } - private String normalizeToolExecutionError(Exception e) { + /** + * Guidance returned to the model when its tool_call arguments JSON was + * truncated or malformed mid-stream. The remedy always comes from the model + * (re-emit a shorter / split call), so the message tells it exactly that — + * without it, models tend to fall back to narrating the result as + * final_answer text. Shared by the JSON pre-validation gate in + * {@link #execute} (which returns this unconditionally — that gate only + * fires for parse failures) and by {@link #normalizeToolExecutionError} + * (which pattern-matches genuine tool-execution errors, which have many + * other causes). + */ + static final String TRUNCATED_ARGS_GUIDANCE = + "Tool execution failed: your tool_call arguments JSON was truncated mid-stream " + + "(very likely you hit max_tokens while emitting a long content field). " + + "Action required: re-call the SAME tool now in your next response, but " + + "(1) make the content field shorter, OR (2) split the work into multiple " + + "sequential tool calls (e.g. write the doc in 2-3 chunks via separate calls). " + + "Do NOT describe the result as text — you must call the tool again to actually " + + "produce the output."; + + static String normalizeToolExecutionError(Exception e) { String message = e != null && e.getMessage() != null ? e.getMessage() : "Unknown error"; String lower = message.toLowerCase(Locale.ROOT); @@ -1187,16 +1215,8 @@ private String normalizeToolExecutionError(Exception e) { // streaming a large `content` field (e.g. renderDocx with 7000+ char // markdown body). The fix MUST come from the model: re-emit the same // tool call with smaller content per call, OR split the work across - // multiple sequential tool calls. We tell the LLM directly so the - // next reasoning iteration knows what to do — without this, models - // tend to fall back to narrating the result as final_answer text. - return "Tool execution failed: your tool_call arguments JSON was truncated mid-stream " - + "(very likely you hit max_tokens while emitting a long content field). " - + "Action required: re-call the SAME tool now in your next response, but " - + "(1) make the content field shorter, OR (2) split the work into multiple " - + "sequential tool calls (e.g. write the doc in 2-3 chunks via separate calls). " - + "Do NOT describe the result as text — you must call the tool again to actually " - + "produce the output."; + // multiple sequential tool calls. + return TRUNCATED_ARGS_GUIDANCE; } if (lower.contains("access denied") && lower.contains("path outside allowed directories")) { diff --git a/mateclaw-server/src/main/java/vip/mate/tool/builtin/CodeExecuteTool.java b/mateclaw-server/src/main/java/vip/mate/tool/builtin/CodeExecuteTool.java index 3aa174f43..9f0e5a1d9 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/builtin/CodeExecuteTool.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/builtin/CodeExecuteTool.java @@ -72,9 +72,14 @@ public class CodeExecuteTool { Use this to act on a skill whose SKILL.md describes steps but ships no runnable script: read the instructions, write the code they describe, and run it here. + IMPORTANT: You MUST pass the complete source code in the "code" parameter. + Never call this tool with "code" missing or empty — it fails with + "No code supplied". Do not describe the code in natural language; write the + actual code and put its full text in "code". + Parameters: - language: one of "python", "bash", "node" - - code: the full source code to run + - code: REQUIRED — the full source code to run (never omit or leave empty) - skillName: optional. When set, the code runs inside that skill's directory (so it can read the skill's reference/template files by relative path) and the skill's stored secrets are injected as environment variables. @@ -95,7 +100,7 @@ public String execute_code( String language, @JsonProperty(required = true) - @JsonPropertyDescription("The full source code to run") + @JsonPropertyDescription("REQUIRED. The full source code to run — never omit or leave empty.") String code, @JsonProperty(required = false) diff --git a/mateclaw-server/src/test/java/vip/mate/agent/graph/NodeStreamingChatHelperToolCallArgsTest.java b/mateclaw-server/src/test/java/vip/mate/agent/graph/NodeStreamingChatHelperToolCallArgsTest.java index 8b10e1acf..3f4175b13 100644 --- a/mateclaw-server/src/test/java/vip/mate/agent/graph/NodeStreamingChatHelperToolCallArgsTest.java +++ b/mateclaw-server/src/test/java/vip/mate/agent/graph/NodeStreamingChatHelperToolCallArgsTest.java @@ -78,11 +78,15 @@ void emptyArguments_replacedWithEmptyJsonObject() { } @Test - @DisplayName("Truncated/invalid JSON arguments normalized to '{}'") - void truncatedJsonArguments_replacedWithEmptyJsonObject() { + @DisplayName("Truncated/invalid JSON arguments pass through raw at aggregation") + void truncatedJsonArguments_passedThroughAtAggregation() { // Simulates a stream cut mid-token: model emitted '{"q":"hel' and stopped. + // At the aggregation point the fragment is passed through verbatim (not + // replaced with '{}') so the tool executor's JSON pre-validation can detect + // the truncation and return the model a "shorten and retry" guidance. + String truncated = "{\"q\":\"hel"; AssistantMessage.ToolCall tc = new AssistantMessage.ToolCall( - "id-truncated", "function", "search", "{\"q\":\"hel"); + "id-truncated", "function", "search", truncated); AssistantMessage msg = AssistantMessage.builder() .content("") .toolCalls(List.of(tc)) @@ -94,9 +98,10 @@ void truncatedJsonArguments_replacedWithEmptyJsonObject() { assertTrue(result.hasToolCalls(), "tool call must survive"); assertEquals(1, result.toolCalls().size()); - assertEquals("{}", result.toolCalls().get(0).arguments(), - "invalid JSON arguments must be replaced with '{}' so the follow-up " - + "request stays well-formed"); + assertEquals(truncated, result.toolCalls().get(0).arguments(), + "a truncated fragment must reach the executor raw so its truncation " + + "detection fires; the outbound chokepoint still normalizes it " + + "to '{}' before the next provider request"); } @Test @@ -147,6 +152,31 @@ void promptHistory_emptyArguments_normalized() { "tool call id must be preserved so the tool_call pairing holds"); } + @Test + @DisplayName("Prompt-history tool call with truncated arguments normalized before send") + void promptHistory_truncatedArguments_normalized() { + // A truncated fragment that survived aggregation (passed through raw so the + // executor could detect it) is persisted into history. On the next send the + // outbound chokepoint must still normalize it to '{}' so strict providers + // (aliyun-codingplan, ...) accept the follow-up request. + AssistantMessage.ToolCall tc = new AssistantMessage.ToolCall( + "id-hist-trunc", "function", "search", "{\"q\":\"hel"); + AssistantMessage historyMsg = AssistantMessage.builder() + .content("calling tool") + .toolCalls(List.of(tc)) + .build(); + Prompt prompt = new Prompt(List.of(new UserMessage("hi"), historyMsg)); + + Prompt normalized = NodeStreamingChatHelper.normalizeToolCallArguments(prompt); + + AssistantMessage out = (AssistantMessage) normalized.getInstructions().get(1); + assertEquals(1, out.getToolCalls().size()); + assertEquals("{}", out.getToolCalls().get(0).arguments(), + "replayed truncated arguments must be normalized to '{}' at the chokepoint"); + assertEquals("id-hist-trunc", out.getToolCalls().get(0).id(), + "tool call id must be preserved so the tool_call pairing holds"); + } + @Test @DisplayName("Prompt with only valid tool-call arguments returned unchanged") void promptHistory_validArguments_returnsSameInstance() { diff --git a/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/ToolExecutionExecutorTruncationGuidanceTest.java b/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/ToolExecutionExecutorTruncationGuidanceTest.java new file mode 100644 index 000000000..da5caa639 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/ToolExecutionExecutorTruncationGuidanceTest.java @@ -0,0 +1,109 @@ +package vip.mate.agent.graph.executor; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Unit coverage for tool-execution error normalization and the truncated-args + * guidance, following the same no-Spring/no-mock style as + * {@link ToolExecutionExecutorCapToolCallsTest}. + * + *

The JSON pre-validation gate in {@code execute()} wraps only + * {@code readTree(arguments)}, so any exception there is a JSON parse failure. + * It returns {@link ToolExecutionExecutor#TRUNCATED_ARGS_GUIDANCE} + * unconditionally rather than pattern-matching the parser message — + * Jackson phrases a mid-token cut as "Unexpected end-of-input" but a malformed + * fragment like {@code {"a":}} as "Unexpected character", and both demand the + * same remedy (re-emit the call). Pattern-matching alone would leave the + * malformed-fragment case with a generic error, so the gate bypasses it. + * {@link ToolExecutionExecutor#normalizeToolExecutionError} still + * pattern-matches for genuine tool-execution errors (the other call sites), + * which have many unrelated causes — those patterns are regression-covered here. + */ +class ToolExecutionExecutorTruncationGuidanceTest { + + // ── The guidance itself ──────────────────────────────────────────────────── + + @Test + @DisplayName("truncation guidance is actionable: names cause + required re-call action") + void guidanceIsActionable() { + String g = ToolExecutionExecutor.TRUNCATED_ARGS_GUIDANCE; + assertTrue(g.contains("truncated mid-stream"), + "must tell the model the args were truncated: " + g); + assertTrue(g.contains("max_tokens"), + "must name the likely cause so the model knows why: " + g); + assertTrue(g.contains("re-call the SAME tool"), + "must instruct the model to re-call the tool, not narrate: " + g); + } + + // ── The pre-validation gate contract ─────────────────────────────────────── + + @Test + @DisplayName("gate guidance covers malformed fragments the pattern table misses") + void gateGuidanceCoversMalformedFragments() { + // A fragment like {"a":} makes Jackson report "Unexpected character ...", + // which is NOT in normalizeToolExecutionError's pattern table — proving + // pattern-matching alone leaves a gap. The gate closes it by returning + // TRUNCATED_ARGS_GUIDANCE unconditionally; assert that guidance is the + // actionable truncation message (not a generic error). + String gateMessage = ToolExecutionExecutor.TRUNCATED_ARGS_GUIDANCE; + assertTrue(gateMessage.contains("re-call the SAME tool"), + "the gate must hand the model the actionable truncation guidance"); + + // Sanity: pattern-matching alone would indeed miss this phrasing, which is + // exactly why the gate does not rely on it. + String unmatched = ToolExecutionExecutor.normalizeToolExecutionError( + new Exception("Unexpected character ('}' (code 125))")); + assertTrue(unmatched.startsWith("Tool execution failed: Unexpected character"), + "pattern-matching falls through to a generic error for this phrasing, " + + "so the gate must not depend on it: " + unmatched); + } + + // ── normalizeToolExecutionError pattern regression (other call sites) ────── + + @Test + @DisplayName("covered JSON-parse messages map to the truncation guidance") + void coveredPatternsReturnGuidance() { + String[] covered = { + "Unexpected end-of-input: was expecting closing quote for a string value", + "Malformed JSON: expected a value", + "JSON parse error: invalid token", + "conversion from JSON failed", + "Unexpected character escape sequence in string", + }; + for (String msg : covered) { + assertSame(ToolExecutionExecutor.TRUNCATED_ARGS_GUIDANCE, + ToolExecutionExecutor.normalizeToolExecutionError(new Exception(msg)), + "message should be treated as truncation: " + msg); + } + } + + @Test + @DisplayName("non-JSON tool error falls through to a generic message") + void genericErrorFallsThrough() { + String out = ToolExecutionExecutor.normalizeToolExecutionError( + new Exception("NullPointerException at line 42")); + assertEquals("Tool execution failed: NullPointerException at line 42", out); + } + + @Test + @DisplayName("workspace path violation maps to the dedicated workspace error") + void accessDeniedReturnsWorkspaceError() { + String out = ToolExecutionExecutor.normalizeToolExecutionError( + new Exception("Access denied: path outside allowed directories (/etc/passwd)")); + assertEquals("Tool execution failed: target path is outside the allowed workspace directory.", out); + } + + @Test + @DisplayName("null exception / null message is handled without NPE") + void nullMessageHandled() { + assertEquals("Tool execution failed: Unknown error", + ToolExecutionExecutor.normalizeToolExecutionError(null)); + assertEquals("Tool execution failed: Unknown error", + ToolExecutionExecutor.normalizeToolExecutionError(new Exception((String) null))); + } +}