From 1ea90c25cb9c624c8bc8eab86de8d4b00703fc90 Mon Sep 17 00:00:00 2001 From: matevip Date: Wed, 13 May 2026 09:03:16 +0800 Subject: [PATCH] =?UTF-8?q?fix(agent):=20make=20head-orphan=20repair=20ord?= =?UTF-8?q?er-sensitive=20=E2=80=94=20a=20later=20same-id=20assistant=20do?= =?UTF-8?q?es=20not=20redeem=20an=20earlier=20orphan?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../main/java/vip/mate/agent/BaseAgent.java | 77 +++++++++++++------ .../agent/BaseAgentHeadOrphanRepairTest.java | 48 +++++++++--- 2 files changed, 91 insertions(+), 34 deletions(-) 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 78d87e93..d36a6810 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/BaseAgent.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/BaseAgent.java @@ -357,10 +357,28 @@ public abstract class BaseAgent { } /** - * 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. + * Drop leading {@link ToolResponseMessage}s whose owning + * {@link AssistantMessage} sits before them in this list. Provider + * validity is order-sensitive: a tool response must follow the assistant + * that issued the tool_call_id; an unrelated later AssistantMessage that + * happens to carry the same id does not redeem an earlier orphan. + * + *

Algorithm: forward scan with a {@code seenIssuedIds} set. Leading + * {@link SystemMessage}s (boundary rows, system prompts) pass through + * untouched but contribute no ids. The first {@link AssistantMessage} or + * {@link UserMessage} we hit stops the repair walk — by that point we're + * out of head-orphan territory. Every {@link ToolResponseMessage} we + * encounter before that stop is checked against {@code seenIssuedIds}; + * if every response id is unseen, the message is dropped and the scan + * re-examines the new head. A response whose ids are all in the seen + * set (e.g. {@code [system, assistant(X), toolResponse(X), ...]} when + * the assistant fell at index 1 of the slice) is left in place. + * + *

Mixed responses (some ids matched, some not) inside a single + * leading {@code ToolResponseMessage} are dropped wholesale rather than + * surgically rewritten — the provider would reject partially-broken + * sequences anyway, and the mixed case implies an upstream invariant + * violation that surfaces in logs. * *

Package-private + static so unit tests can drive it without standing * up a full BaseAgent subclass. @@ -368,17 +386,11 @@ public abstract class BaseAgent { 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()); - } - } - } - } + // Built up as we walk; only assistants we've already passed count + // toward "preceding". An assistant that sits behind a head orphan is + // irrelevant: provider order-validity asks "was this tool_call id + // issued BEFORE this response?", not "anywhere in the prompt". + Set seenIssuedIds = new HashSet<>(); int dropped = 0; int i = 0; @@ -390,25 +402,44 @@ public abstract class BaseAgent { i++; continue; } + if (m instanceof AssistantMessage am) { + // Reached a preceding assistant — head danger is over. The + // tool_call ids it issued are valid for any tool responses + // that follow, but we stop the repair walk here either way. + if (am.getToolCalls() != null) { + for (AssistantMessage.ToolCall tc : am.getToolCalls()) { + if (tc.id() != null && !tc.id().isEmpty()) { + seenIssuedIds.add(tc.id()); + } + } + } + break; + } if (m instanceof ToolResponseMessage trm) { - boolean allOrphan = trm.getResponses().stream() + // Every response id must have been issued by a preceding + // assistant we already walked through. If any single id is + // missing from seenIssuedIds, the message is invalid in + // place. Empty / null ids don't count for or against. + boolean anyUnmatched = trm.getResponses().stream() .map(ToolResponseMessage.ToolResponse::id) .filter(id -> id != null && !id.isEmpty()) - .allMatch(id -> !issuedIds.contains(id)); - if (allOrphan) { + .anyMatch(id -> !seenIssuedIds.contains(id)); + if (anyUnmatched) { messages.remove(i); dropped++; continue; // re-examine the new messages[i] } + // All ids match a preceding assistant — keep, and stop the + // repair walk. Anything past here is well-formed by + // construction (provider validates each subsequent pair as + // we go). + break; } - // 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. + // UserMessage (or anything else) — past the head danger. Stop. break; } if (dropped > 0) { - log.info("[{}] Stripped {} leading orphan ToolResponseMessage(s) — owning AssistantMessage was outside the recent window", + log.info("[{}] Stripped {} leading orphan ToolResponseMessage(s) — no preceding AssistantMessage in scope", agentName, dropped); } return dropped; diff --git a/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java b/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java index 13400023..3fad6503 100644 --- a/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java +++ b/mateclaw-server/src/test/java/vip/mate/agent/BaseAgentHeadOrphanRepairTest.java @@ -110,13 +110,38 @@ class BaseAgentHeadOrphanRepairTest { } @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. + void laterAssistantWithSameIdDoesNotRedeemHeadOrphan() { + // The classic order-sensitivity trap: a ToolResponseMessage sits at + // the head, and a LATER AssistantMessage happens to carry the same + // tool_call_id. The provider's contract is "tool_call must precede + // tool_response", not "tool_call exists somewhere in the prompt". + // The leading response is therefore still orphan and must be dropped. + List messages = new ArrayList<>(List.of( + new SystemMessage("[boundary]"), + toolResponse("call-X"), + new UserMessage("hi"), + assistantWithToolCalls("call-X"), // same id, but AFTER the response + toolResponse("call-X") + )); + + int dropped = BaseAgent.stripHeadOrphanToolResponses(messages, "test"); + + assertEquals(1, dropped, + "the leading response is orphan regardless of whether a later assistant " + + "happens to carry the same id — provider validity is order-sensitive"); + assertInstanceOf(SystemMessage.class, messages.get(0)); + assertInstanceOf(UserMessage.class, messages.get(1), + "the orphan that sat between the boundary and the user turn is gone"); + } + + @Test + void partialOrphanInLeadingResponseIsDropped() { + // A ToolResponseMessage with two responses — one whose id has no + // preceding assistant, one whose id has none either (since we + // haven't walked any assistants yet). Provider order-validity + // doesn't allow partial pairs; dropping wholesale is the safer + // call. We lose matched-response content but never emit a request + // the provider would 400. ToolResponseMessage mixed = ToolResponseMessage.builder().responses(List.of( new ToolResponseMessage.ToolResponse("call-orphan", "tool_x", "x"), new ToolResponseMessage.ToolResponse("call-known", "tool_y", "y") @@ -129,10 +154,11 @@ class BaseAgentHeadOrphanRepairTest { 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()); + assertEquals(1, dropped, + "no preceding assistant has been walked yet, so even a partially-matched " + + "leading response is dropped wholesale"); + assertInstanceOf(AssistantMessage.class, messages.getFirst(), + "the mixed head is gone; the assistant that would have owned call-known is now first"); } @Test