diff --git a/mateclaw-server/src/main/java/vip/mate/agent/BaseAgent.java b/mateclaw-server/src/main/java/vip/mate/agent/BaseAgent.java index bee8e7a1..78d87e93 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/BaseAgent.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/BaseAgent.java @@ -15,6 +15,7 @@ import vip.mate.llm.routing.MediaCaptionService; import vip.mate.llm.routing.MultimodalRouter; import vip.mate.llm.routing.model.MultimodalRoutingDecision; import vip.mate.llm.service.ModelCapabilityService; +import org.springframework.ai.chat.messages.ToolResponseMessage; import vip.mate.workspace.conversation.ConversationService; import vip.mate.workspace.conversation.model.MessageContentPart; import vip.mate.workspace.conversation.model.MessageEntity; @@ -24,6 +25,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; import java.util.EnumSet; +import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; @@ -328,9 +330,90 @@ public abstract class BaseAgent { while (!messages.isEmpty() && messages.get(messages.size() - 1) instanceof UserMessage) { messages.remove(messages.size() - 1); } + + // Head guard — orphan tool-response strip. + // + // Independent of the compaction pair-safe boundary in + // ConversationWindowManager: that one protects the *compaction* cut, + // this one protects the *pagination* cut. listRecentMessages returns + // the last N rows verbatim, and the first row of that page can be a + // ToolResponseMessage whose owning AssistantMessage sat one row + // earlier — i.e. outside the page. Sending such a sequence to any + // OpenAI-compatible provider returns 400 because every tool response + // must be preceded by an assistant message issuing that tool_call_id. + // + // The boundary prepend earlier inserts a SystemMessage at the head; + // the orphan, if present, sits at index 1 in that case. Skip leading + // SystemMessages and drop any ToolResponseMessage whose response ids + // are all unmatched by the AssistantMessages still in scope. + // + // Dropping is correct rather than expanding backward to fetch the + // missing assistant: if the AssistantMessage is outside the window, + // its content is already lost to the model anyway, and the boundary + // summary (if any) covers it. Keeping the orphan would just trade a + // dropped row for a 400. + stripHeadOrphanToolResponses(messages, agentName); return messages; } + /** + * Drop leading {@link ToolResponseMessage}s whose response ids cannot be + * matched against any {@link AssistantMessage} tool_call id in the list. + * Leaves any leading {@link SystemMessage}s (boundary rows, system prompts) + * intact and continues scanning past them. + * + *

Package-private + static so unit tests can drive it without standing + * up a full BaseAgent subclass. + */ + static int stripHeadOrphanToolResponses(List messages, String agentName) { + if (messages.isEmpty()) return 0; + + // Collect every tool_call id issued by any AssistantMessage in scope. + Set issuedIds = new HashSet<>(); + for (Message m : messages) { + if (m instanceof AssistantMessage am && am.getToolCalls() != null) { + for (AssistantMessage.ToolCall tc : am.getToolCalls()) { + if (tc.id() != null && !tc.id().isEmpty()) { + issuedIds.add(tc.id()); + } + } + } + } + + int dropped = 0; + int i = 0; + while (i < messages.size()) { + Message m = messages.get(i); + if (m instanceof SystemMessage) { + // Boundary rows / system prompts pass through; advance and + // keep looking for orphan tool responses that sit behind them. + i++; + continue; + } + if (m instanceof ToolResponseMessage trm) { + boolean allOrphan = trm.getResponses().stream() + .map(ToolResponseMessage.ToolResponse::id) + .filter(id -> id != null && !id.isEmpty()) + .allMatch(id -> !issuedIds.contains(id)); + if (allOrphan) { + messages.remove(i); + dropped++; + continue; // re-examine the new messages[i] + } + } + // First non-orphan, non-system message — stop. Deeper orphans are + // upstream bugs (every other call path keeps pairs together); + // dropping aggressively from here on would mask those instead of + // surfacing them in logs. + break; + } + if (dropped > 0) { + log.info("[{}] Stripped {} leading orphan ToolResponseMessage(s) — owning AssistantMessage was outside the recent window", + agentName, dropped); + } + return dropped; + } + /** * History sanitization entry point. Encapsulates *all* steps applied to a * persisted message before it reaches an LLM prompt. Returns {@code null} diff --git a/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java b/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java new file mode 100644 index 00000000..13400023 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java @@ -0,0 +1,198 @@ +package vip.mate.agent; + +import org.junit.jupiter.api.Test; +import org.springframework.ai.chat.messages.AssistantMessage; +import org.springframework.ai.chat.messages.Message; +import org.springframework.ai.chat.messages.SystemMessage; +import org.springframework.ai.chat.messages.ToolResponseMessage; +import org.springframework.ai.chat.messages.UserMessage; + +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Head-side pair repair on the recent-message pagination cut. + * + *

{@code listRecentMessages(conversationId, windowSize)} returns the last N + * rows verbatim. The first row of that page can be a {@link ToolResponseMessage} + * whose owning {@link AssistantMessage} (carrying the matching tool_call_id) + * sat one row earlier — i.e. outside the page. Sending such a sequence to any + * OpenAI-compatible provider returns 400 because every tool response must be + * preceded by an assistant message issuing that tool_call_id. + * + *

{@link BaseAgent#stripHeadOrphanToolResponses} drops leading + * {@code ToolResponseMessage}s whose response ids are unmatched by every + * AssistantMessage still in scope. {@link SystemMessage}s (boundary rows, + * system prompts) at the head are skipped over, not removed. + */ +class BaseAgentHeadOrphanRepairTest { + + @Test + void orphanToolResponseAtHeadIsDropped() { + // Window starts with a TOOL response (orphan: no AssistantMessage in this list issued call-X). + List messages = new ArrayList<>(List.of( + toolResponse("call-X"), + new UserMessage("next user turn"), + assistantWithToolCalls("call-Y"), + toolResponse("call-Y") + )); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(1, dropped, "leading orphan should be dropped"); + assertInstanceOf(UserMessage.class, messages.getFirst(), + "head is now the user turn, not the orphan tool response"); + } + + @Test + void multipleConsecutiveOrphansAtHeadAllDropped() { + // A single AssistantMessage outside the window may have produced + // several tool calls whose responses landed in two separate + // ToolResponseMessages. Both should be removed. + List messages = new ArrayList<>(List.of( + toolResponse("call-A"), + toolResponse("call-B"), + new UserMessage("here we go"), + assistantWithToolCalls("call-C"), + toolResponse("call-C") + )); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(2, dropped); + assertInstanceOf(UserMessage.class, messages.getFirst()); + } + + @Test + void systemBoundaryAtHeadIsSkippedAndOrphanBehindItIsDropped() { + // After findLatestCompressionBoundary prepends a SystemMessage, the + // orphan tool response now sits at index 1. The repair must skip the + // system row and still drop the orphan. + SystemMessage boundary = new SystemMessage("[compression boundary placeholder]"); + List messages = new ArrayList<>(List.of( + boundary, + toolResponse("call-X"), + new UserMessage("after orphan"), + assistantWithToolCalls("call-Y"), + toolResponse("call-Y") + )); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(1, dropped); + assertSame(boundary, messages.getFirst(), + "the system boundary stays in place"); + assertInstanceOf(UserMessage.class, messages.get(1), + "the orphan that sat behind the boundary is gone"); + } + + @Test + void matchedHeadToolResponseIsKept() { + // The window happens to start with both the AssistantMessage and its + // tool response — perfectly aligned, nothing to drop. + List messages = new ArrayList<>(List.of( + assistantWithToolCalls("call-A"), + toolResponse("call-A"), + new UserMessage("next") + )); + List snapshot = new ArrayList<>(messages); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(0, dropped); + assertEquals(snapshot, messages, "no drops, list unchanged"); + } + + @Test + void partialOrphanIsKeptToSurfaceUpstreamBug() { + // A ToolResponseMessage with two responses — one orphan, one matched + // in scope. Real pipelines should never produce this (each + // ToolResponseMessage closes ONE assistant turn) but if it happens, + // dropping the whole message would also lose the matched response. + // Stop at the first non-orphan and let the upstream invariant + // violation surface in logs. + ToolResponseMessage mixed = ToolResponseMessage.builder().responses(List.of( + new ToolResponseMessage.ToolResponse("call-orphan", "tool_x", "x"), + new ToolResponseMessage.ToolResponse("call-known", "tool_y", "y") + )).build(); + List messages = new ArrayList<>(List.of( + mixed, + assistantWithToolCalls("call-known"), + toolResponse("call-known") + )); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(0, dropped, + "mixed orphan/matched responses in one message are NOT dropped — " + + "the partial-match case is an upstream bug we want to see in logs"); + assertSame(mixed, messages.getFirst()); + } + + @Test + void emptyListIsNoOp() { + List messages = new ArrayList<>(); + assertEquals(0, BaseAgent.stripHeadOrphanToolResponses(messages, "test")); + assertTrue(messages.isEmpty()); + } + + @Test + void purelyUserAssistantHistoryUntouched() { + // No tool responses at all — repair is a no-op. + List messages = new ArrayList<>(List.of( + new UserMessage("hi"), + new AssistantMessage("hello"), + new UserMessage("how are you?"), + new AssistantMessage("good") + )); + List snapshot = new ArrayList<>(messages); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(0, dropped); + assertEquals(snapshot, messages); + } + + @Test + void stopsAtFirstNonOrphanNonSystem() { + // Once we hit a non-system, non-orphan message, repair stops — we do + // NOT keep walking and look for orphans deeper in the history. + // Deeper orphans imply an upstream bug; this guard is only here to + // protect the pagination cut. + List messages = new ArrayList<>(List.of( + toolResponse("call-A"), // orphan at head — will be dropped + new UserMessage("user"), // stops the scan + toolResponse("call-B"), // orphan but we do NOT touch it + new AssistantMessage("late") + )); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(1, dropped); + assertInstanceOf(UserMessage.class, messages.getFirst()); + assertFalse(messages.stream().noneMatch(m -> m instanceof ToolResponseMessage), + "the deeper orphan stays in place — it surfaces as an upstream bug elsewhere"); + } + + // ------------------------------------------------------------------ helpers + + private static AssistantMessage assistantWithToolCalls(String... callIds) { + List calls = new ArrayList<>(); + for (String id : callIds) { + calls.add(new AssistantMessage.ToolCall(id, "function", "tool_" + id, "{}")); + } + return AssistantMessage.builder().content("").toolCalls(calls).build(); + } + + private static ToolResponseMessage toolResponse(String callId) { + return ToolResponseMessage.builder().responses(List.of( + new ToolResponseMessage.ToolResponse(callId, "tool_" + callId, "ok") + )).build(); + } +}