diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 37de1c682..d0d0660b9 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -228,16 +228,10 @@ func (cb *ContextBuilder) BuildMessages(history []providers.Message, summary str systemPrompt += "\n\n## Summary of Previous Conversation\n\n" + summary } - //This fix prevents the session memory from LLM failure due to elimination of toolu_IDs required from LLM - // --- INICIO DEL FIX --- - //Diegox-17 - for len(history) > 0 && (history[0].Role == "tool") { - logger.DebugCF("agent", "Removing orphaned tool message from history to prevent LLM error", - map[string]interface{}{"role": history[0].Role}) - history = history[1:] - } - //Diegox-17 - // --- FIN DEL FIX --- + // Sanitize tool messages: remove orphaned tool responses and strip + // tool_calls whose responses are missing. This prevents API errors + // after context compression or mid-execution cancellation. + history = sanitizeToolMessages(history) messages = append(messages, providers.Message{ Role: "system", @@ -309,3 +303,67 @@ func (cb *ContextBuilder) GetSkillsInfo() map[string]interface{} { "names": skillNames, } } + +// sanitizeToolMessages ensures tool_calls and tool responses are consistent. +// It performs two passes: +// 1. Remove orphaned tool messages whose matching tool_calls assistant is missing. +// 2. Strip individual tool_calls entries from assistant messages when the +// corresponding tool response is missing (e.g. cancelled mid-execution). +// +// This prevents API errors like "messages with role 'tool' must be a response +// to a preceding message with 'tool_calls'" after compression or cancellation. +func sanitizeToolMessages(history []providers.Message) []providers.Message { + // Pass 1: collect IDs present in each direction + toolCallIDs := make(map[string]bool) // IDs declared by assistant tool_calls + toolRespIDs := make(map[string]bool) // IDs present as tool responses + + for _, msg := range history { + if msg.Role == "assistant" && len(msg.ToolCalls) > 0 { + for _, tc := range msg.ToolCalls { + toolCallIDs[tc.ID] = true + } + } + if msg.Role == "tool" && msg.ToolCallID != "" { + toolRespIDs[msg.ToolCallID] = true + } + } + + // Pass 2: rebuild history with fixes + result := make([]providers.Message, 0, len(history)) + for _, msg := range history { + // Remove orphaned tool responses (no matching tool_calls) + if msg.Role == "tool" && msg.ToolCallID != "" { + if !toolCallIDs[msg.ToolCallID] { + logger.DebugCF("agent", "Removing orphaned tool message", map[string]interface{}{ + "tool_call_id": msg.ToolCallID, + }) + continue + } + } + + // Strip tool_calls entries that have no corresponding tool response + if msg.Role == "assistant" && len(msg.ToolCalls) > 0 { + kept := make([]providers.ToolCall, 0, len(msg.ToolCalls)) + for _, tc := range msg.ToolCalls { + if toolRespIDs[tc.ID] { + kept = append(kept, tc) + } else { + logger.DebugCF("agent", "Removing tool_call with missing response", map[string]interface{}{ + "tool_call_id": tc.ID, + }) + } + } + if len(kept) != len(msg.ToolCalls) { + // Create a copy so we don't mutate the original slice + fixed := msg + fixed.ToolCalls = kept + result = append(result, fixed) + continue + } + } + + result = append(result, msg) + } + + return result +} diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index fbc4713d6..dd247b973 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -637,6 +637,14 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.M } errMsg := strings.ToLower(err.Error()) + + // Request cancellation (e.g. user sent a new message) is not a context + // window error — break out of the retry loop immediately. + // The caller's ctx.Err() check (line ~530) will handle the cancel gracefully. + if ctx.Err() != nil { + break + } + // Check for context window errors (provider specific, but usually contain "token" or "invalid") isContextError := strings.Contains(errMsg, "token") || strings.Contains(errMsg, "context") || @@ -966,14 +974,33 @@ func (al *AgentLoop) forceCompression(sessionKey string) { // Helper to find the mid-point of the conversation mid := len(conversation) / 2 - // New history structure: - // 1. System Prompt - // 2. [Summary of dropped part] - synthesized - // 3. Second half of conversation - // 4. Last message + // Adjust mid so we don't split in the middle of a tool_calls/tool group. + // Both cases move mid forward to include the entire group in the dropped + // portion, which maximises context freed by forceCompression. + // + // Case 1: mid lands on a "tool" message — advance past all consecutive + // tool messages so the group is dropped together with its + // preceding assistant (already in the dropped half). + // Case 2: mid lands on an "assistant" with tool_calls — advance past + // the subsequent tool responses so the whole group is dropped. + if mid < len(conversation) && conversation[mid].Role == "tool" { + for mid < len(conversation) && conversation[mid].Role == "tool" { + mid++ + } + } else if mid < len(conversation) && conversation[mid].Role == "assistant" && len(conversation[mid].ToolCalls) > 0 { + mid++ + for mid < len(conversation) && conversation[mid].Role == "tool" { + mid++ + } + } - // Simplified approach for emergency: Drop first half of conversation - // and rely on existing summary if present, or create a placeholder. + // Clamp: ensure we keep at least something and drop at least something + if mid <= 0 { + mid = 1 + } + if mid >= len(conversation) { + mid = len(conversation) - 1 + } droppedCount := mid keptConversation := conversation[mid:] @@ -987,9 +1014,10 @@ func (al *AgentLoop) forceCompression(sessionKey string) { // The summary is stored separately in session.Summary, so it persists! // We just need to ensure the user knows there's a gap. - // We only modify the messages list here + // Use "user" role for the compression note because some providers reject + // "system" messages that appear after the initial system prompt. newHistory = append(newHistory, providers.Message{ - Role: "system", + Role: "user", Content: compressionNote, }) diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index d51cdf95a..b82ebedef 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "time" @@ -843,6 +844,7 @@ func TestAgentLoop_ContextExhaustionRetry(t *testing.T) { // Inject some history to simulate a full context sessionKey := "test-session-context" + al.sessions.GetOrCreate(sessionKey) // Create dummy history history := []providers.Message{ {Role: "system", Content: "System prompt"}, @@ -882,3 +884,428 @@ func TestAgentLoop_ContextExhaustionRetry(t *testing.T) { t.Errorf("Expected history to be compressed (len < 8), got %d", len(finalHistory)) } } + +// cancelDuringChatProvider cancels the given context inside Chat() and returns +// a "context canceled" error, simulating an HTTP request aborted mid-flight. +type cancelDuringChatProvider struct { + cancelFn context.CancelFunc // called inside Chat to simulate cancellation + currentCall int +} + +func (m *cancelDuringChatProvider) Chat(ctx context.Context, messages []providers.Message, tools []providers.ToolDefinition, model string, opts map[string]interface{}) (*providers.LLMResponse, error) { + m.currentCall++ + // Cancel the context from within Chat, then return the error the HTTP + // client would produce when its request context is cancelled. + m.cancelFn() + return nil, fmt.Errorf("Post \"https://api.example.com\": context canceled") +} + +func (m *cancelDuringChatProvider) GetDefaultModel() string { + return "mock-cancel-model" +} + +// TestRetryLoop_CancelledContextSkipsCompression verifies that when +// provider.Chat() returns "context canceled" because the request was aborted, +// the retry loop detects ctx.Err() and breaks WITHOUT triggering forceCompression. +func TestRetryLoop_CancelledContextSkipsCompression(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + DataDir: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + + // Create a cancellable context — the provider will cancel it during Chat() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + provider := &cancelDuringChatProvider{cancelFn: cancel} + + al := NewAgentLoop(cfg, msgBus, provider) + + // Set up session with enough history for forceCompression to act on + sessionKey := "test-cancel-session" + al.sessions.GetOrCreate(sessionKey) + history := []providers.Message{ + {Role: "system", Content: "System prompt"}, + {Role: "user", Content: "Old message 1"}, + {Role: "assistant", Content: "Old response 1"}, + {Role: "user", Content: "Old message 2"}, + {Role: "assistant", Content: "Old response 2"}, + {Role: "user", Content: "Old message 3"}, + {Role: "assistant", Content: "Old response 3"}, + {Role: "user", Content: "Trigger message"}, + } + al.sessions.SetHistory(sessionKey, history) + + _, _ = al.ProcessDirectWithChannel(ctx, "Trigger message", sessionKey, "test", "test-chat") + + // History must NOT contain a compression note — forceCompression should not have run + finalHistory := al.sessions.GetHistory(sessionKey) + for _, msg := range finalHistory { + if strings.Contains(msg.Content, "Emergency compression") { + t.Error("Context cancellation must NOT trigger forceCompression") + } + } + + // Provider.Chat() should have been called exactly once (no retry) + if provider.currentCall != 1 { + t.Errorf("Expected exactly 1 provider call (no retry), got %d", provider.currentCall) + } +} + +// TestForceCompression_ToolGroupBoundary verifies forceCompression adjusts the +// split point to avoid breaking tool_calls/tool response groups. +func TestForceCompression_ToolGroupBoundary(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + DataDir: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + sessionKey := "test-compression-boundary" + al.sessions.GetOrCreate(sessionKey) + + // Build history where naive mid would land on a tool message. + // Layout: [system, user, assistant+tool_calls, tool, tool, user, assistant, user] + // mid of conversation (indices 1-6, len=7) = 3, which is a "tool" message. + history := []providers.Message{ + {Role: "system", Content: "System prompt"}, + {Role: "user", Content: "msg 1"}, + {Role: "assistant", Content: "thinking", ToolCalls: []providers.ToolCall{ + {ID: "tc1", Type: "function", Function: &providers.FunctionCall{Name: "read", Arguments: "{}"}}, + {ID: "tc2", Type: "function", Function: &providers.FunctionCall{Name: "write", Arguments: "{}"}}, + }}, + {Role: "tool", Content: "result1", ToolCallID: "tc1"}, + {Role: "tool", Content: "result2", ToolCallID: "tc2"}, + {Role: "user", Content: "msg 2"}, + {Role: "assistant", Content: "response 2"}, + {Role: "user", Content: "msg 3"}, + } + + al.sessions.SetHistory(sessionKey, history) + al.forceCompression(sessionKey) + + result := al.sessions.GetHistory(sessionKey) + + // Verify no orphaned tool messages remain + toolCallIDs := make(map[string]bool) + for _, msg := range result { + if msg.Role == "assistant" { + for _, tc := range msg.ToolCalls { + toolCallIDs[tc.ID] = true + } + } + } + for _, msg := range result { + if msg.Role == "tool" && msg.ToolCallID != "" { + if !toolCallIDs[msg.ToolCallID] { + t.Errorf("Orphaned tool message found: ToolCallID=%s", msg.ToolCallID) + } + } + } + + // Verify no assistant with tool_calls has missing tool responses + toolRespIDs := make(map[string]bool) + for _, msg := range result { + if msg.Role == "tool" && msg.ToolCallID != "" { + toolRespIDs[msg.ToolCallID] = true + } + } + for _, msg := range result { + if msg.Role == "assistant" { + for _, tc := range msg.ToolCalls { + if !toolRespIDs[tc.ID] { + t.Errorf("Assistant has tool_call %s but no tool response found", tc.ID) + } + } + } + } +} + +// TestForceCompression_MidOnAssistantWithToolCalls verifies that when mid lands +// on an assistant message with tool_calls, the subsequent tool responses are +// also dropped together. +func TestForceCompression_MidOnAssistantWithToolCalls(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + DataDir: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + sessionKey := "test-compression-assistant-mid" + al.sessions.GetOrCreate(sessionKey) + + // Layout: [system, user, assistant, user, assistant+tool_calls, tool, user, assistant, user, assistant, user] + // conversation (indices 1-9, len=10), mid=5 which is "tool" + // Actually let's make mid land on assistant+tool_calls: + // conversation len=8, mid=4 → assistant+tool_calls + history := []providers.Message{ + {Role: "system", Content: "System prompt"}, + {Role: "user", Content: "msg 1"}, + {Role: "assistant", Content: "resp 1"}, + {Role: "user", Content: "msg 2"}, + {Role: "assistant", Content: "resp 2"}, + // mid=4 of conversation lands here: + {Role: "assistant", Content: "thinking", ToolCalls: []providers.ToolCall{ + {ID: "tc1", Type: "function", Function: &providers.FunctionCall{Name: "read", Arguments: "{}"}}, + }}, + {Role: "tool", Content: "file contents", ToolCallID: "tc1"}, + {Role: "user", Content: "msg 3"}, + {Role: "assistant", Content: "resp 3"}, + {Role: "user", Content: "final msg"}, + } + + al.sessions.SetHistory(sessionKey, history) + al.forceCompression(sessionKey) + + result := al.sessions.GetHistory(sessionKey) + + // Verify integrity + toolCallIDs := make(map[string]bool) + toolRespIDs := make(map[string]bool) + for _, msg := range result { + if msg.Role == "assistant" { + for _, tc := range msg.ToolCalls { + toolCallIDs[tc.ID] = true + } + } + if msg.Role == "tool" && msg.ToolCallID != "" { + toolRespIDs[msg.ToolCallID] = true + } + } + + // Every tool response must have a matching tool_call + for _, msg := range result { + if msg.Role == "tool" && msg.ToolCallID != "" { + if !toolCallIDs[msg.ToolCallID] { + t.Errorf("Orphaned tool message: ToolCallID=%s", msg.ToolCallID) + } + } + } + + // Every tool_call must have a matching response + for _, msg := range result { + if msg.Role == "assistant" { + for _, tc := range msg.ToolCalls { + if !toolRespIDs[tc.ID] { + t.Errorf("Missing tool response for tool_call %s", tc.ID) + } + } + } + } +} + +// TestForceCompression_NoteUsesUserRole verifies the compression note uses "user" role +func TestForceCompression_NoteUsesUserRole(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "agent-test-*") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + DataDir: tmpDir, + Model: "test-model", + MaxTokens: 4096, + MaxToolIterations: 10, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &mockProvider{} + al := NewAgentLoop(cfg, msgBus, provider) + + sessionKey := "test-compression-role" + al.sessions.GetOrCreate(sessionKey) + history := []providers.Message{ + {Role: "system", Content: "System prompt"}, + {Role: "user", Content: "msg 1"}, + {Role: "assistant", Content: "resp 1"}, + {Role: "user", Content: "msg 2"}, + {Role: "assistant", Content: "resp 2"}, + {Role: "user", Content: "msg 3"}, + {Role: "assistant", Content: "resp 3"}, + {Role: "user", Content: "final"}, + } + + al.sessions.SetHistory(sessionKey, history) + al.forceCompression(sessionKey) + + result := al.sessions.GetHistory(sessionKey) + + // Find the compression note and verify it uses "user" role + found := false + for _, msg := range result { + if strings.Contains(msg.Content, "Emergency compression") { + found = true + if msg.Role != "user" { + t.Errorf("Compression note should use 'user' role, got '%s'", msg.Role) + } + } + } + if !found { + t.Error("Expected to find compression note in history") + } +} + +// TestSanitizeToolMessages_OrphanedToolRemoval verifies orphaned tool messages are removed +func TestSanitizeToolMessages_OrphanedToolRemoval(t *testing.T) { + history := []providers.Message{ + {Role: "user", Content: "hello"}, + // Orphaned tool message — no preceding assistant with tool_calls + {Role: "tool", Content: "orphaned result", ToolCallID: "tc-orphan"}, + {Role: "assistant", Content: "response"}, + } + + result := sanitizeToolMessages(history) + + for _, msg := range result { + if msg.Role == "tool" { + t.Error("Orphaned tool message should have been removed") + } + } + + if len(result) != 2 { + t.Errorf("Expected 2 messages after sanitization, got %d", len(result)) + } +} + +// TestSanitizeToolMessages_MissingResponseStripped verifies tool_calls with missing +// responses are stripped from assistant messages +func TestSanitizeToolMessages_MissingResponseStripped(t *testing.T) { + history := []providers.Message{ + {Role: "user", Content: "do something"}, + {Role: "assistant", Content: "calling tools", ToolCalls: []providers.ToolCall{ + {ID: "tc1", Type: "function", Function: &providers.FunctionCall{Name: "read", Arguments: "{}"}}, + {ID: "tc2", Type: "function", Function: &providers.FunctionCall{Name: "write", Arguments: "{}"}}, + {ID: "tc3", Type: "function", Function: &providers.FunctionCall{Name: "exec", Arguments: "{}"}}, + }}, + // Only tc1 has a response — tc2 and tc3 were cancelled mid-execution + {Role: "tool", Content: "read result", ToolCallID: "tc1"}, + {Role: "assistant", Content: "done"}, + } + + result := sanitizeToolMessages(history) + + // Find the assistant message and verify only tc1 remains + for _, msg := range result { + if msg.Role == "assistant" && len(msg.ToolCalls) > 0 { + if len(msg.ToolCalls) != 1 { + t.Errorf("Expected 1 tool_call after sanitization, got %d", len(msg.ToolCalls)) + } + if msg.ToolCalls[0].ID != "tc1" { + t.Errorf("Expected remaining tool_call to be tc1, got %s", msg.ToolCalls[0].ID) + } + // Content should be preserved + if msg.Content != "calling tools" { + t.Errorf("Assistant content should be preserved, got '%s'", msg.Content) + } + } + } +} + +// TestSanitizeToolMessages_CompleteGroupPreserved verifies that complete +// tool_calls/tool groups are kept intact +func TestSanitizeToolMessages_CompleteGroupPreserved(t *testing.T) { + history := []providers.Message{ + {Role: "user", Content: "do something"}, + {Role: "assistant", Content: "calling tools", ToolCalls: []providers.ToolCall{ + {ID: "tc1", Type: "function", Function: &providers.FunctionCall{Name: "read", Arguments: "{}"}}, + {ID: "tc2", Type: "function", Function: &providers.FunctionCall{Name: "write", Arguments: "{}"}}, + }}, + {Role: "tool", Content: "read result", ToolCallID: "tc1"}, + {Role: "tool", Content: "write result", ToolCallID: "tc2"}, + {Role: "assistant", Content: "done"}, + } + + result := sanitizeToolMessages(history) + + // Everything should be preserved + if len(result) != len(history) { + t.Errorf("Complete group should be preserved; expected %d messages, got %d", + len(history), len(result)) + } + + // Verify tool_calls are intact + for _, msg := range result { + if msg.Role == "assistant" && len(msg.ToolCalls) > 0 { + if len(msg.ToolCalls) != 2 { + t.Errorf("Expected 2 tool_calls preserved, got %d", len(msg.ToolCalls)) + } + } + } +} + +// TestSanitizeToolMessages_AllToolCallsMissing verifies that when ALL tool_calls +// have missing responses, the ToolCalls slice becomes empty +func TestSanitizeToolMessages_AllToolCallsMissing(t *testing.T) { + history := []providers.Message{ + {Role: "user", Content: "do something"}, + {Role: "assistant", Content: "calling tools", ToolCalls: []providers.ToolCall{ + {ID: "tc1", Type: "function", Function: &providers.FunctionCall{Name: "read", Arguments: "{}"}}, + }}, + // No tool response at all — cancelled immediately + {Role: "assistant", Content: "never mind"}, + } + + result := sanitizeToolMessages(history) + + // The assistant message should have empty ToolCalls + for _, msg := range result { + if msg.Role == "assistant" && msg.Content == "calling tools" { + if len(msg.ToolCalls) != 0 { + t.Errorf("Expected 0 tool_calls after sanitization, got %d", len(msg.ToolCalls)) + } + } + } +}