From 8f49b0ec92b9adbfec853518d4fb78c97f1c2075 Mon Sep 17 00:00:00 2001 From: matevip Date: Wed, 13 May 2026 08:49:56 +0800 Subject: [PATCH] fix(executor): spill raw tool result before falling back to inline truncate --- .../graph/executor/ToolExecutionExecutor.java | 109 +++++++++---- .../graph/executor/ToolResultProperties.java | 24 ++- .../src/main/resources/application.yml | 16 +- .../executor/LaneDExecutorAndConfigTest.java | 145 ++++++++++++++++-- 4 files changed, 232 insertions(+), 62 deletions(-) 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 c0db1130..aba6c73a 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 @@ -79,22 +79,70 @@ public class ToolExecutionExecutor { ); /** - * Layer 1 — hard truncation cap applied to every tool result before it - * reaches ToolResultStorage (Layer 2 spill) or the LLM prompt. + * Inline hard-truncate cap for a single tool result. Acts as the fallback + * when raw-first spill cannot run (storage disabled, tool excluded, body + * already at-or-below the spill threshold, or disk write failed). * - *

Two-level budget chain (RFC-008 / RFC-06 D-5): + *

Per-tool-result handling chain: * 

-     *   raw tool result
-     *     → truncateToolResult(..., MAX_TOOL_RESULT_CHARS=8000)   // Layer 1: hard cap
-     *     → persistIfOversized(..., perResultThresholdChars=16000) // Layer 2: spill to disk
-     *     → enforceTurnBudget(..., perTurnBudgetChars=32000)      // Layer 3: per-turn aggregate
+     *   raw tool result (full bytes)
+     *     → spillRawOrTruncate(...)
+     *         ├─ persistIfOversized(...) tries to write the raw body to disk
+     *         │     when size > perResultThresholdChars and tool is not
+     *         │     in the spill exclusion list. Returns a SPILL_MARKER preview
+     *         │     on success, or the original string otherwise.
+     *         └─ if no SPILL_MARKER on the return, truncateToolResult(...)
+     *               caps inline to MAX_TOOL_RESULT_CHARS so a multi-MB raw
+     *               body never enters the model prompt.
+     *     → enforceTurnBudget(..., perTurnBudgetChars=32000)   // per-turn aggregate
      *
- * Layer 1 runs first and is intentionally kept at 8000 to prevent oversized - * results from inflating the prompt. Layers 2/3 thresholds are configured in - * {@link ToolResultProperties} and application.yml. + * Spill must see the RAW result so the full output is preserved on disk + * and the model can call {@code read_file} on the spill path. Truncating + * before spilling would write a pre-shortened blob to disk, defeating the + * "ground truth on disk" guarantee. {@link ToolResultProperties} controls + * the thresholds; this constant stays in code because it is the safety + * net for the failure case and should not vary by deployment. */ private static final int MAX_TOOL_RESULT_CHARS = 8000; + /** + * Raw-first spill: try to write the full result to disk via the spill + * store; only fall back to inline hard-truncate when no spill marker + * comes back. Caller distinguishes spill success from "returned + * unchanged" by checking {@link ToolResultStorage#SPILL_MARKER_PREFIX} + * on the returned string — otherwise an IO failure or under-threshold + * body would slip through indistinguishable from a successful spill, + * and a multi-MB raw body could end up in the model prompt. + * + *

Package-private + static so the spill/truncate decision is unit + * testable in isolation from the rest of the executor. + * + * @param storage spill store; {@code null} skips the spill attempt + * @param maxTruncateChars fallback inline hard cap + * @param result raw tool output (may be {@code null}) + * @param toolName used in the spill preview header + * @param toolUseId unique within the conversation; becomes the file name + * @param conversationId spill files are scoped per conversation; blank/null falls back to "unknown" + * @param workspaceBasePath where the spill directory lives when set + * @return the SPILL_MARKER preview when spill succeeded, otherwise the + * original string (when ≤ threshold) or the inline-truncated string. + */ + static String spillRawOrTruncate(ToolResultStorage storage, int maxTruncateChars, + String result, String toolName, String toolUseId, + String conversationId, String workspaceBasePath) { + if (result == null) return null; + if (storage != null) { + String safeConv = conversationId != null && !conversationId.isEmpty() + ? conversationId : "unknown"; + String candidate = storage.persistIfOversized( + result, toolName, toolUseId, safeConv, workspaceBasePath); + if (candidate != null && candidate.startsWith(ToolResultStorage.SPILL_MARKER_PREFIX)) { + return candidate; + } + } + return truncateToolResult(result, maxTruncateChars); + } + /** 尾部错误模式检测 */ private static final java.util.regex.Pattern ERROR_TAIL_PATTERN = java.util.regex.Pattern.compile( "(?i)\\b(error|exception|traceback|failed|fatal|panic|stack.?trace|errno)\\b"); @@ -581,16 +629,13 @@ public class ToolExecutionExecutor { toolCall.id(), toolName, DIRECT_TOOL_PLACEHOLDER); } - // RFC-008 Layer 1 first, then Layer 2 — match the non-replay path - // in executeSingleTool so behavior stays symmetric across approval - // replays. The caller-supplied conversationId scopes spill files - // into the same per-conversation directory layout. - result = truncateToolResult(result, MAX_TOOL_RESULT_CHARS); - if (resultStorage != null && result != null) { - String spillConv = conversationId != null && !conversationId.isEmpty() ? conversationId : "unknown"; - result = resultStorage.persistIfOversized( - result, toolName, toolCall.id(), spillConv, workspaceBasePath); - } + // Raw-first spill, inline truncate as fallback. Symmetric with the + // non-replay path in executeSingleTool. The caller-supplied + // conversationId scopes spill files into the per-conversation + // directory layout. See spillRawOrTruncate javadoc for why the + // order matters. + result = spillRawOrTruncate(resultStorage, MAX_TOOL_RESULT_CHARS, + result, toolName, toolCall.id(), conversationId, workspaceBasePath); log.info("[ToolExecutor] Pre-approved tool {} returned {} chars{}", toolName, rawLen, result != null && result.length() < rawLen ? " (now " + result.length() + " after spill/truncate)" : ""); events.add(GraphEventPublisher.toolComplete(toolCall.id(), toolName, result, true)); @@ -806,20 +851,16 @@ public class ToolExecutionExecutor { } } - // RFC-008 Layer 1: hard truncation cap to prevent oversized results - // from inflating the prompt. Runs FIRST (before spill) so the spill - // store doesn't need to handle multi-MB writes for run-of-the-mill - // greps that happen to spit out a long stdout. - result = truncateToolResult(result, MAX_TOOL_RESULT_CHARS); - // RFC-008 Layer 2: spill oversized results to disk and replace - // with preview + path. Falls back to truncation when spilling is - // disabled or fails. Spill preserves the full output (read_file can - // retrieve it); the Layer 1 truncation above already capped the - // inline portion, so this layer mostly catches near-cap residues. - if (resultStorage != null && result != null) { - result = resultStorage.persistIfOversized( - result, toolName, pc.toolCall.id(), pc.conversationId, pc.workspaceBasePath); - } + // Raw-first spill: write the full output to disk and replace + // with preview + path so the model can call read_file for the + // ground truth. Fall back to inline truncate only when spilling + // is disabled, the tool is on the exclusion list, the body is + // already under the spill threshold, or the disk write fails. + // Truncating before spilling would persist a pre-shortened body + // to disk and silently lose data the model could otherwise + // recover. + result = spillRawOrTruncate(resultStorage, MAX_TOOL_RESULT_CHARS, + result, toolName, pc.toolCall.id(), pc.conversationId, pc.workspaceBasePath); log.info("[ToolExecutor] Tool {} returned {} chars{}", toolName, rawLen, result != null && result.length() < rawLen ? " (now " + result.length() + " after spill/truncate)" : ""); events.add(GraphEventPublisher.toolComplete(pc.toolCall.id(), toolName, result, true)); diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultProperties.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultProperties.java index d4c90dd4..41bb473d 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultProperties.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultProperties.java @@ -40,12 +40,26 @@ public class ToolResultProperties { private boolean enabled = true; /** - * Layer 2 — a single tool result larger than this is spilled to disk. - * The executor evaluates this against the raw result before applying the - * final inline cap, so oversized content is preserved before it is shortened - * for the model request. + * Per-result spill threshold. A single tool result larger than this is + * spilled to disk and the in-context view is replaced with a short + * preview + path so the model can call {@code read_file} on demand. + * + *

Aligned with {@code ToolExecutionExecutor.MAX_TOOL_RESULT_CHARS} + * (8000): the executor now tries to spill the RAW result first; only + * when spilling is disabled, the tool is on {@link #excludedTools}, the + * body is under this threshold, or the disk write fails, does it fall + * back to truncating inline to 8000 chars. Keeping the threshold equal + * to the truncate cap yields a single semantic ladder — above the + * threshold means "preserved on disk", at-or-below means "stays inline + * verbatim". + * + *

If you want to keep more text inline before spilling, raise this + * value AND raise the executor's hard cap together; otherwise the + * 8000-char fallback truncate would silently shorten anything between + * this threshold and 8000 even when spill is disabled, defeating the + * intent. */ - private int perResultThresholdChars = 16000; // was 4000 — prevents WebSearch spill-to-disk + private int perResultThresholdChars = 8000; /** * Layer 3 — aggregate cap on combined response size in one tool turn. diff --git a/mateclaw-server/src/main/resources/application.yml b/mateclaw-server/src/main/resources/application.yml index 1778126c..73da99a1 100644 --- a/mateclaw-server/src/main/resources/application.yml +++ b/mateclaw-server/src/main/resources/application.yml @@ -207,15 +207,17 @@ mate: per-category: shell: 120 web: 30 - # RFC-008 Phase 3: tool-result three-layer budget (per-result spill + per-turn aggregate budget). - # Layer 1 (per-tool cap) lives inside individual tools; Layer 2 spills oversized - # single results to disk; Layer 3 enforces an aggregate cap on the combined - # response size of one tool turn. The full output is preserved on disk and - # the in-context preview points the agent at the spill file (read_file tool). + # Tool-result budget (per-result spill + per-turn aggregate budget). + # The executor tries to spill the RAW result first so the full output is + # preserved on disk; the in-context preview points the agent at the spill + # file via read_file. When spill is disabled, the tool is on the exclusion + # list, the body is at or below the threshold, or the disk write fails, + # the executor falls back to inline hard-truncation to the same character + # cap. Per-turn aggregate caps the combined size across one tool turn. tool-result: enabled: true - per-result-threshold-chars: 16000 # was 4000 — prevents WebSearch spill-to-disk - per-turn-budget-chars: 32000 # was 16000 — headroom for multi-tool turns + per-result-threshold-chars: 8000 # aligned with executor hard cap; > this size → spill, ≤ → inline verbatim + per-turn-budget-chars: 32000 # headroom for multi-tool turns preview-head-chars: 800 excluded-tool-inline-chars: 2500 storage-base-dir: "" diff --git a/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/LaneDExecutorAndConfigTest.java b/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/LaneDExecutorAndConfigTest.java index e1a13147..a13047d9 100644 --- a/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/LaneDExecutorAndConfigTest.java +++ b/mateclaw-server/src/test/java/vip/mate/agent/graph/executor/LaneDExecutorAndConfigTest.java @@ -14,11 +14,14 @@ import java.util.concurrent.ExecutorService; import static org.junit.jupiter.api.Assertions.*; /** - * Tests for Lane D executor and config changes: + * Tests for the tool-result executor pipeline and its associated config. * *

*/ class LaneDExecutorAndConfigTest { @@ -68,19 +71,19 @@ class LaneDExecutorAndConfigTest { // ============================================================ @Nested - @DisplayName("D-5: ToolResultProperties defaults updated") + @DisplayName("ToolResultProperties defaults") class ToolResultPropertiesDefaultsTests { @Test - @DisplayName("perResultThresholdChars default is 16000 (was 4000)") + @DisplayName("perResultThresholdChars default aligns with executor hard cap (8000)") void perResultThresholdCharsDefault() { ToolResultProperties props = new ToolResultProperties(); - assertEquals(16000, props.getPerResultThresholdChars(), - "Default perResultThresholdChars should be 16000"); + assertEquals(8000, props.getPerResultThresholdChars(), + "Default perResultThresholdChars should equal the executor's MAX_TOOL_RESULT_CHARS=8000"); } @Test - @DisplayName("perTurnBudgetChars default is 32000 (was 16000)") + @DisplayName("perTurnBudgetChars default is 32000") void perTurnBudgetCharsDefault() { ToolResultProperties props = new ToolResultProperties(); assertEquals(32000, props.getPerTurnBudgetChars(), @@ -101,15 +104,125 @@ class LaneDExecutorAndConfigTest { } @Test - @DisplayName("Large results above 16000 still trigger spill (threshold boundary)") - void thresholdBoundary() { + @DisplayName("Per-result threshold matches the executor inline hard cap so spill and truncate share one ladder") + void thresholdMatchesExecutorHardCap() throws Exception { ToolResultProperties props = new ToolResultProperties(); - // Results <= 16000 should NOT spill - assertTrue(15000 <= props.getPerResultThresholdChars(), - "A 15000-char result should be within threshold"); - // Results > 16000 should spill - assertTrue(17000 > props.getPerResultThresholdChars(), - "A 17000-char result should exceed threshold"); + Field field = ToolExecutionExecutor.class.getDeclaredField("MAX_TOOL_RESULT_CHARS"); + field.setAccessible(true); + int hardCap = (int) field.get(null); + assertEquals(hardCap, props.getPerResultThresholdChars(), + "perResultThresholdChars must equal MAX_TOOL_RESULT_CHARS; misalignment would silently shorten " + + "bodies between the two values when spill is disabled."); + } + } + + // ============================================================ + // Raw-first spill ordering — the critical issue #110 fix + // ============================================================ + + @Nested + @DisplayName("ToolExecutionExecutor.spillRawOrTruncate: raw body reaches disk before the inline cap") + class SpillOrTruncateOrderingTests { + + @TempDir + Path tempDir; + + private ToolResultStorage storage(int threshold, List excluded) { + ToolResultProperties props = new ToolResultProperties(); + props.setStorageBaseDir(tempDir.toString()); + props.setPerResultThresholdChars(threshold); + props.setPreviewHeadChars(120); + if (excluded != null) props.setExcludedTools(excluded); + return new ToolResultStorage(props); + } + + @Test + @DisplayName("raw body > threshold → spill writes full original bytes to disk and returns preview") + void rawOverThresholdSpillsFullContent() throws Exception { + ToolResultStorage st = storage(1000, null); + String raw = "0123456789\n".repeat(2000); // ~22000 chars, well over both threshold AND hard cap + int rawLen = raw.length(); + + String out = ToolExecutionExecutor.spillRawOrTruncate( + st, 8000, raw, "web_search", "call-1", "conv-x", tempDir.toString()); + + assertTrue(out.startsWith(ToolResultStorage.SPILL_MARKER_PREFIX), + "should return a spill preview when raw exceeds threshold"); + assertTrue(out.contains("full_chars=" + rawLen), + "preview header must report the original size, proving the raw bytes were what we measured"); + + // The file on disk should be the FULL raw body — not the 8000-char truncate. + // Path is encoded inside the preview as "path=/abs/path". + java.util.regex.Matcher m = java.util.regex.Pattern.compile("path=(\\S+)").matcher(out); + assertTrue(m.find(), "preview must include path=..."); + Path spillFile = Path.of(m.group(1)); + assertTrue(java.nio.file.Files.exists(spillFile), "spill file should have been created"); + String fileContent = java.nio.file.Files.readString(spillFile); + assertEquals(rawLen, fileContent.length(), + "spill file must contain the full raw body, not a pre-truncated copy"); + } + + @Test + @DisplayName("raw body > threshold but tool is on exclusion list → no spill, inline hard cap") + void rawOverThresholdExcludedToolTruncatesOnly() { + ToolResultStorage st = storage(1000, List.of("read_file")); + String raw = "x".repeat(20000); + + String out = ToolExecutionExecutor.spillRawOrTruncate( + st, 8000, raw, "read_file", "call-1", "conv-x", tempDir.toString()); + + assertFalse(out.startsWith(ToolResultStorage.SPILL_MARKER_PREFIX), + "excluded tool must not be spilled"); + assertTrue(out.length() <= 8000, + "excluded body still must fit the inline hard cap (was " + out.length() + ")"); + } + + @Test + @DisplayName("raw body ≤ threshold → returned unchanged, no spill, no truncation marker added") + void rawUnderThresholdInlineVerbatim() { + ToolResultStorage st = storage(1000, null); + String raw = "small body"; + + String out = ToolExecutionExecutor.spillRawOrTruncate( + st, 8000, raw, "web_search", "call-1", "conv-x", tempDir.toString()); + + assertEquals(raw, out, "small bodies should pass through untouched"); + } + + @Test + @DisplayName("storage null → falls back to inline hard cap, never crashes") + void nullStorageFallsBackToTruncate() { + String raw = "x".repeat(20000); + + String out = ToolExecutionExecutor.spillRawOrTruncate( + null, 8000, raw, "web_search", "call-1", "conv-x", tempDir.toString()); + + assertFalse(out.startsWith(ToolResultStorage.SPILL_MARKER_PREFIX)); + assertTrue(out.length() <= 8000, + "with no storage, body must still fit the inline hard cap"); + } + + @Test + @DisplayName("null result stays null (no NPE)") + void nullResultStaysNull() { + ToolResultStorage st = storage(1000, null); + String out = ToolExecutionExecutor.spillRawOrTruncate( + st, 8000, null, "web_search", "call-1", "conv-x", tempDir.toString()); + assertNull(out); + } + + @Test + @DisplayName("blank conversationId is replaced with a safe 'unknown' bucket so spill still lands on disk") + void blankConversationIdRoutesToUnknownBucket() { + ToolResultStorage st = storage(1000, null); + String raw = "y".repeat(20000); + + String out = ToolExecutionExecutor.spillRawOrTruncate( + st, 8000, raw, "web_search", "call-1", "", tempDir.toString()); + + assertTrue(out.startsWith(ToolResultStorage.SPILL_MARKER_PREFIX), + "blank conversationId must not stop spill — caller can be the legacy executePreApproved path"); + assertTrue(out.contains("unknown"), "spill path should land under the 'unknown' bucket"); } }