From 3b1c1ba6c18e8ce0292cd8f6e3868873a2f58239 Mon Sep 17 00:00:00 2001 From: Hung Nguyen Date: Mon, 17 Aug 2026 19:34:13 +0700 Subject: [PATCH] fix(agent): balance history when a wave aborts mid-flight The anti-loop fatal path saved the assistant message with its tool_calls and no tool results, discarding results that had already completed. --- pkg/agent/loop_fatal_balance_test.go | 139 +++++++++++++++++++++++++++ pkg/agent/loop_iteration.go | 7 ++ pkg/agent/loop_state.go | 27 ++++++ 3 files changed, 173 insertions(+) create mode 100644 pkg/agent/loop_fatal_balance_test.go diff --git a/pkg/agent/loop_fatal_balance_test.go b/pkg/agent/loop_fatal_balance_test.go new file mode 100644 index 0000000..032d1f4 --- /dev/null +++ b/pkg/agent/loop_fatal_balance_test.go @@ -0,0 +1,139 @@ +package agent + +import ( + "context" + "testing" + + "github.com/hung12ct/gopheragent/pkg/history" +) + +// repeatCalls builds n tool calls that are byte-identical except for their +// provider call ID, so the anti-loop detector trips partway through the wave +// while the calls that already ran hold real results. +func repeatCalls(n int) []PendingToolCall { + out := make([]PendingToolCall, n) + for i := range n { + out[i] = PendingToolCall{ + ID: "dup" + string(rune('0'+i)), + Name: "counter", + ArgsJSON: `{"same":true}`, + } + } + return out +} + +// assertToolCallsBalanced fails when any tool_call in msgs lacks a tool +// message carrying the matching ToolCallID. Providers reject an unbalanced +// transcript, so this is the property the saved history must always hold. +func assertToolCallsBalanced(t *testing.T, msgs []history.Message) { + t.Helper() + replied := make(map[string]bool) + for _, m := range msgs { + if m.Role == "tool" && m.ToolCallID != "" { + replied[m.ToolCallID] = true + } + } + for _, m := range msgs { + if m.Role != "assistant" { + continue + } + for _, tc := range m.ToolCalls { + if !replied[tc.ID] { + t.Fatalf("tool_call %q has no matching tool result in saved history (%d messages)", tc.ID, len(msgs)) + } + } + } +} + +// A wave aborted by the anti-loop detector used to save the assistant +// message with its tool_calls and no tool results at all, discarding the +// results that had already completed. The saved transcript must instead be +// balanced, with completed results preserved. +func TestFatalWave_SavesBalancedHistory(t *testing.T) { + ct := &countingTool{name: "counter"} + provider := &scriptProvider{turns: []LLMResult{ + {ToolCalls: repeatCalls(6)}, + {Content: "final"}, + }} + loop, sm := setup(provider, ct) + + if _, err := loop.RunIteration(context.Background(), "s1", "go"); err == nil { + t.Fatal("expected the anti-loop detector to abort the turn") + } + + msgs, err := sm.History(context.Background(), "s1") + if err != nil { + t.Fatalf("history: %v", err) + } + assertToolCallsBalanced(t, msgs) + + // The abort must not throw away work that already landed: at least one + // call completed before the detector tripped at loopKillThreshold. + var real, synthetic int + for _, m := range msgs { + if m.Role != "tool" { + continue + } + if m.Content == fatalAbortToolReason { + synthetic++ + continue + } + real++ + } + if real == 0 { + t.Fatalf("every tool result was synthetic; completed work was discarded (%d synthetic)", synthetic) + } + if got := real + synthetic; got != 6 { + t.Fatalf("tool results = %d, want 6 (real=%d synthetic=%d)", got, real, synthetic) + } +} + +// synthesizeMissingToolErrors must never overwrite a recorded result. +func TestSynthesizeMissingToolErrors_PreservesCompleted(t *testing.T) { + calls := repeatCalls(3) + toolMsgs := map[string]history.Message{ + calls[1].ID: {Role: "tool", Content: "kept", ToolCallID: calls[1].ID}, + } + + synthesizeMissingToolErrors(toolMsgs, calls, fatalAbortToolReason) + + if len(toolMsgs) != 3 { + t.Fatalf("len(toolMsgs) = %d, want 3", len(toolMsgs)) + } + if got := toolMsgs[calls[1].ID].Content; got != "kept" { + t.Fatalf("completed result overwritten: %q", got) + } + for _, i := range []int{0, 2} { + m := toolMsgs[calls[i].ID] + if m.Content != fatalAbortToolReason || !m.IsError || m.ToolCallID != calls[i].ID { + t.Fatalf("call %d: unexpected synthetic message %+v", i, m) + } + } +} + +// An empty per-wave map is the abort-before-any-dispatch case: every call +// still needs a reply. +func TestSynthesizeMissingToolErrors_FillsEmptyMap(t *testing.T) { + calls := repeatCalls(4) + toolMsgs := map[string]history.Message{} + + synthesizeMissingToolErrors(toolMsgs, calls, fatalAbortToolReason) + + if len(toolMsgs) != len(calls) { + t.Fatalf("len(toolMsgs) = %d, want %d", len(toolMsgs), len(calls)) + } + var msgs []history.Message + msgs = append(msgs, history.Message{Role: "assistant", ToolCalls: toolCallsOf(calls)}) + msgs = appendToolResultsInOrder(msgs, calls, toolMsgs) + assertToolCallsBalanced(t, msgs) +} + +// toolCallsOf mirrors the pending calls into the history shape the assistant +// message carries, so a balance assertion can run over a hand-built slice. +func toolCallsOf(calls []PendingToolCall) []history.ToolCall { + out := make([]history.ToolCall, 0, len(calls)) + for _, tc := range calls { + out = append(out, history.ToolCall{ID: tc.ID, Name: tc.Name, Arguments: tc.ArgsJSON}) + } + return out +} diff --git a/pkg/agent/loop_iteration.go b/pkg/agent/loop_iteration.go index c89a39a..e69f529 100644 --- a/pkg/agent/loop_iteration.go +++ b/pkg/agent/loop_iteration.go @@ -96,6 +96,13 @@ func (al *AgentLoop) runIteration(ctx context.Context, sessionKey string, stream ws := al.executeToolWaves(ctx, st, scheduled) if ws.fatalErr != nil { iterErr = ws.fatalErr + // The assistant message persisted above carries tool_calls, so the + // transcript is only valid once every call has a tool reply. Keep the + // results that completed before the abort (real work, and the looping + // call's own result is what the detector tripped on) and account for + // the rest, so the saved history needs no downstream repair. + synthesizeMissingToolErrors(ws.toolMsgs, result.ToolCalls, fatalAbortToolReason) + *msgs = appendToolResultsInOrder(*msgs, result.ToolCalls, ws.toolMsgs) al.saveSession(ctx, sessionKey, *msgs) al.emit(ctx, sessionKey, streamChan, errEvent(fmt.Errorf("%w: %w", ErrLoopDetected, ws.fatalErr))) return len(scheduled), true diff --git a/pkg/agent/loop_state.go b/pkg/agent/loop_state.go index 7fbfdd0..a09cd19 100644 --- a/pkg/agent/loop_state.go +++ b/pkg/agent/loop_state.go @@ -139,3 +139,30 @@ func synthesizeDroppedToolErrors(toolMsgs map[string]history.Message, dropped [] } } } + +// fatalAbortToolReason is the tool-result content recorded for a call the +// loop abandoned when a wave aborted. It names the cause and tells the +// model the call never ran, so a resumed session does not read the gap as +// a silent success. +const fatalAbortToolReason = "tools: not executed — the turn was aborted mid-wave after a repeated-call loop was detected; do not repeat the same call." + +// synthesizeMissingToolErrors fills every call in calls that has no +// recorded result with a tool-error message naming reason, leaving +// completed results untouched. An assistant message carrying tool_calls +// is only a valid transcript when every call has a matching tool reply, +// so an abort that skips calls must still account for them. Same +// no-lock precondition as synthesizeDroppedToolErrors: every wave +// goroutine has returned by the time this runs. +func synthesizeMissingToolErrors(toolMsgs map[string]history.Message, calls []PendingToolCall, reason string) { + for _, tc := range calls { + if m, ok := toolMsgs[tc.ID]; ok && m.Role != "" { + continue + } + toolMsgs[tc.ID] = history.Message{ + Role: "tool", + Content: reason, + ToolCallID: tc.ID, + IsError: true, + } + } +}