From 8ecd3c029f1da7e643e62004f840d57eb8f898d0 Mon Sep 17 00:00:00 2001 From: daotri Date: Tue, 1 Sep 2026 03:54:08 +0700 Subject: [PATCH] fix(agent): repair transcript pairing after compaction and truncation (#1470) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mid-loop compaction output now runs through sanitizeHistory so orphaned tool messages never reach the provider (OpenAI rejects role:tool without a preceding assistant tool_calls). Background summarization truncates on tool-chain boundaries via the shared toolChainSplitIndex helper so the kept tail never starts mid tool-chain. Co-authored-by: Trí Đào Nguyễn Minh --- internal/agent/loop_compact.go | 34 +-- internal/agent/loop_compact_boundary_test.go | 205 +++++++++++++++++++ internal/agent/loop_history_sanitize.go | 14 +- internal/agent/loop_pipeline_callbacks.go | 7 + 4 files changed, 246 insertions(+), 14 deletions(-) create mode 100644 internal/agent/loop_compact_boundary_test.go diff --git a/internal/agent/loop_compact.go b/internal/agent/loop_compact.go index 7adb6382..8ee15293 100644 --- a/internal/agent/loop_compact.go +++ b/internal/agent/loop_compact.go @@ -56,18 +56,7 @@ func (l *Loop) compactMessagesInPlace(ctx context.Context, messages []providers. keepCount = minKeep } - splitIdx := len(messages) - keepCount - - // Walk backward from splitIdx to find a clean boundary — - // avoid splitting tool_use → tool_result pairs. - for splitIdx > 0 { - m := messages[splitIdx] - if m.Role == "tool" || (m.Role == "assistant" && len(m.ToolCalls) > 0) { - splitIdx-- - continue - } - break - } + splitIdx := toolChainSplitIndex(messages, len(messages)-keepCount) if splitIdx <= 1 { return nil } @@ -133,6 +122,27 @@ func (l *Loop) compactMessagesInPlace(ctx context.Context, messages []providers. return result } +// toolChainSplitIndex walks backward from splitIdx to the nearest clean +// boundary so the kept tail (messages[splitIdx:]) never starts mid tool-chain. +// A tail starting with a tool result — or splitting an assistant(tool_calls) → +// tool sequence — produces an orphaned role:"tool" message, which OpenAI +// rejects with a 400 on the next request. Used by both mid-loop compaction +// and the background summarizer's truncation. +func toolChainSplitIndex(messages []providers.Message, splitIdx int) int { + if splitIdx > len(messages) { + splitIdx = len(messages) + } + for splitIdx > 0 && splitIdx < len(messages) { + m := messages[splitIdx] + if m.Role == "tool" || (m.Role == "assistant" && len(m.ToolCalls) > 0) { + splitIdx-- + continue + } + break + } + return splitIdx +} + // dynamicSummaryMax returns the output-token budget for a compaction or // summarization call, scaled to input size. Formula: in/25 (~4% compression), // clamped to [1024, 8192]. Floor keeps short summaries coherent; cap prevents diff --git a/internal/agent/loop_compact_boundary_test.go b/internal/agent/loop_compact_boundary_test.go new file mode 100644 index 00000000..98b49ef2 --- /dev/null +++ b/internal/agent/loop_compact_boundary_test.go @@ -0,0 +1,205 @@ +package agent + +import ( + "context" + "testing" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/providers" +) + +// TestToolChainSplitIndex verifies the shared boundary walk used by mid-loop +// compaction and the background summarizer: the kept tail (messages[idx:]) +// must never start with a tool result or split an assistant(tool_calls) → +// tool chain, because providers reject orphaned role:"tool" messages. +func TestToolChainSplitIndex(t *testing.T) { + user := providers.Message{Role: "user", Content: "u"} + assistant := providers.Message{Role: "assistant", Content: "a"} + assistantTC := providers.Message{ + Role: "assistant", + Content: "calling tool", + ToolCalls: []providers.ToolCall{{ID: "tc-1"}}, + } + tool := providers.Message{Role: "tool", Content: "result", ToolCallID: "tc-1"} + + tests := []struct { + name string + messages []providers.Message + splitIdx int + want int + }{ + { + name: "clean boundary on user message stays put", + messages: []providers.Message{user, assistant, user, assistant}, + splitIdx: 2, + want: 2, + }, + { + name: "split on tool result walks back before assistant tool_calls", + messages: []providers.Message{user, assistant, assistantTC, tool, user}, + splitIdx: 3, + want: 1, + }, + { + name: "split on assistant with tool_calls walks back", + messages: []providers.Message{user, assistant, assistantTC, tool, user}, + splitIdx: 2, + want: 1, + }, + { + name: "consecutive tool results walk back through whole chain", + messages: []providers.Message{user, assistantTC, tool, tool, user}, + splitIdx: 3, + want: 0, + }, + { + name: "split at zero unchanged", + messages: []providers.Message{user, assistant}, + splitIdx: 0, + want: 0, + }, + { + name: "split at len keeps empty tail unchanged", + messages: []providers.Message{user, assistantTC, tool}, + splitIdx: 3, + want: 3, + }, + { + name: "split beyond len clamps to len", + messages: []providers.Message{user, assistant}, + splitIdx: 5, + want: 2, + }, + { + name: "chain reaching history start returns zero", + messages: []providers.Message{assistantTC, tool, user}, + splitIdx: 1, + want: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := toolChainSplitIndex(tt.messages, tt.splitIdx) + if got != tt.want { + t.Fatalf("toolChainSplitIndex() = %d, want %d", got, tt.want) + } + // Invariant: the kept tail never starts mid tool-chain. + if got > 0 && got < len(tt.messages) { + m := tt.messages[got] + if m.Role == "tool" { + t.Errorf("kept tail starts with role=tool at index %d", got) + } + if m.Role == "assistant" && len(m.ToolCalls) > 0 { + t.Errorf("kept tail starts on assistant(tool_calls) at index %d", got) + } + } + }) + } +} + +// TestMakeCompactMessages_SanitizesOrphanTool verifies the compaction callback +// repairs pairing on the compacted slice: an orphaned tool message surviving +// inside the kept tail must be dropped before the slice replaces run history. +func TestMakeCompactMessages_SanitizesOrphanTool(t *testing.T) { + cap := &capturingProvider{response: "Summary of conversation."} + loop := &Loop{ + provider: cap, + model: "claude-3-5-sonnet", + } + + // 10 messages → keepCount=4, splitIdx=6. messages[6] is a plain user + // message (clean boundary), so the orphan tool at index 7 lands in the + // kept tail and must be repaired by the sanitize pass. + msgs := []providers.Message{ + {Role: "user", Content: "u0"}, + {Role: "assistant", Content: "a1"}, + {Role: "user", Content: "u2"}, + {Role: "assistant", Content: "a3"}, + {Role: "user", Content: "u4"}, + {Role: "assistant", Content: "a5"}, + {Role: "user", Content: "u6"}, + {Role: "tool", Content: "orphaned result", ToolCallID: "orphan-1"}, + {Role: "user", Content: "u8"}, + {Role: "assistant", Content: "a9"}, + } + + compact := loop.makeCompactMessages(nil) + result, err := compact(context.Background(), msgs, loop.model) + if err != nil { + t.Fatalf("compact callback error: %v", err) + } + if len(result) == 0 { + t.Fatal("compact callback returned empty messages") + } + if len(cap.captured) != 1 { + t.Fatalf("provider.Chat called %d time(s), want 1 (compaction must have run)", len(cap.captured)) + } + for i, m := range result { + if m.Role == "tool" { + t.Errorf("orphaned tool message survived at index %d (tool_call_id=%s)", i, m.ToolCallID) + } + } +} + +// truncCapturingStore records the keepLast argument of TruncateHistory calls. +type truncCapturingStore struct { + nopSessionStore + truncated chan int +} + +func (s *truncCapturingStore) TruncateHistory(_ context.Context, _ string, keepLast int) { + select { + case s.truncated <- keepLast: + default: + } +} + +// TestMaybeSummarize_ToolChainBoundary verifies the background summarizer +// widens the kept tail when the raw split (len-keepLast) lands mid tool-chain, +// so truncation never persists a history starting with an orphaned tool message. +func TestMaybeSummarize_ToolChainBoundary(t *testing.T) { + const contextWindow = 10000 + + // threshold = 10000 * 0.85 = 8500 tokens; long content pushes the + // estimate over it so summarization triggers. + longContent := makeLongString(9000) + history := []providers.Message{ + {Role: "user", Content: longContent}, // 0 + {Role: "assistant", Content: longContent}, // 1 + {Role: "user", Content: longContent}, // 2 + {Role: "assistant", Content: longContent}, // 3 + {Role: "assistant", Content: longContent, // 4 + ToolCalls: []providers.ToolCall{{ID: "tc-a"}, {ID: "tc-b"}}}, + {Role: "tool", Content: "result a", ToolCallID: "tc-a"}, // 5 + {Role: "tool", Content: "result b", ToolCallID: "tc-b"}, // 6 + {Role: "user", Content: longContent}, // 7 + {Role: "assistant", Content: longContent}, // 8 + {Role: "user", Content: longContent}, // 9 + } + // keepLast=4 → raw split at index 6 (a tool result). The boundary walk + // must retreat to index 3, keeping the last 7 messages. + const wantKeep = 7 + + store := &truncCapturingStore{ + nopSessionStore: nopSessionStore{history: history}, + truncated: make(chan int, 1), + } + loop := &Loop{ + provider: &capturingProvider{response: "summary"}, + model: "claude-3-5-sonnet", + contextWindow: contextWindow, + sessions: store, + } + + loop.maybeSummarize(context.Background(), "test-session-key") + + select { + case got := <-store.truncated: + if got != wantKeep { + t.Fatalf("TruncateHistory keepLast = %d, want %d (kept tail must not start mid tool-chain)", got, wantKeep) + } + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for maybeSummarize to truncate history") + } +} diff --git a/internal/agent/loop_history_sanitize.go b/internal/agent/loop_history_sanitize.go index 706f0dc9..514a1153 100644 --- a/internal/agent/loop_history_sanitize.go +++ b/internal/agent/loop_history_sanitize.go @@ -242,8 +242,18 @@ func (l *Loop) maybeSummarize(ctx context.Context, sessionKey string) { return } + // Adjust the split so the kept tail never starts mid tool-chain — + // a tail beginning with a tool result (or splitting assistant(tool_calls) + // from its results) persists an orphaned role:"tool" message that + // OpenAI rejects with a 400 on the next request. + splitIdx := toolChainSplitIndex(history, len(history)-keepLast) + if splitIdx <= 0 { + return + } + effectiveKeep := len(history) - splitIdx + summary := l.sessions.GetSummary(sctx, sessionKey) - toSummarize := history[:len(history)-keepLast] + toSummarize := history[:splitIdx] var sb strings.Builder var mediaKinds []string @@ -308,7 +318,7 @@ func (l *Loop) maybeSummarize(ctx context.Context, sessionKey string) { } l.sessions.SetSummary(sctx, sessionKey, SanitizeAssistantContent(resp.Content)) - l.sessions.TruncateHistory(sctx, sessionKey, keepLast) + l.sessions.TruncateHistory(sctx, sessionKey, effectiveKeep) // Inject preserved MediaRefs into the first kept message so they survive truncation. if len(preservedRefs) > 0 { diff --git a/internal/agent/loop_pipeline_callbacks.go b/internal/agent/loop_pipeline_callbacks.go index 39b84398..84a8f3eb 100644 --- a/internal/agent/loop_pipeline_callbacks.go +++ b/internal/agent/loop_pipeline_callbacks.go @@ -560,6 +560,13 @@ func (l *Loop) makeCompactMessages(req *RunRequest) func(ctx context.Context, ms if compacted == nil { return msgs, nil // compaction failed, return original } + // The compacted slice replaces run history and is persisted verbatim — + // repair tool_use/tool_result pairing now so an orphaned role:"tool" + // message never reaches the provider (OpenAI rejects it with a 400). + compacted, repaired := sanitizeHistory(compacted) + if repaired > 0 { + slog.Warn("post_compaction_sanitize", "agent", l.id, "repaired", repaired) + } // Stamp session metadata with the compaction timestamp so operators // can diagnose compaction cadence without a dedicated column. Stored // as RFC3339 string in sessions.metadata JSONB (flushed on next save).