diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 26abe9079a..46fd05b9eb 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -121,7 +121,13 @@ type continuationTarget struct { const ( defaultResponse = "The model returned an empty response. This may indicate a provider error or token limit." toolLimitResponse = "I've reached `max_tool_iterations` without a final response. Increase `max_tool_iterations` in config.json if this task needs more tool steps." - handledToolResponseSummary = "Requested output delivered via tool attachment." + // repeatedFailureThreshold is the number of consecutive identical + // tool+error failures after which the turn is stopped early with a + // user-visible explanation instead of spinning silently to + // max_tool_iterations (see issue: unanswered messages). + repeatedFailureThreshold = 3 + repeatedFailureResponse = "I kept hitting the same error while using the `%s` tool (%d times in a row):\n\n%s\n\nStopping here instead of retrying the same failing call. If the task needs this tool, fix the underlying problem and try again." + handledToolResponseSummary = "Requested output delivered via tool attachment." sessionKeyAgentPrefix = "agent:" pendingTurnPrefix = "pending-" providerReloadGracePeriod = 30 * time.Second diff --git a/pkg/agent/tool_repeat_failure_test.go b/pkg/agent/tool_repeat_failure_test.go new file mode 100644 index 0000000000..5be9b78243 --- /dev/null +++ b/pkg/agent/tool_repeat_failure_test.go @@ -0,0 +1,181 @@ +package agent + +import ( + "context" + "sync" + "testing" + + "github.com/sipeed/picoclaw/pkg/bus" + "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/providers" + "github.com/sipeed/picoclaw/pkg/routing" + "github.com/sipeed/picoclaw/pkg/session" + "github.com/sipeed/picoclaw/pkg/tools" +) + +// alwaysFailingToolProvider simulates an LLM that keeps requesting the same +// tool call over and over and never produces a final answer. This mirrors a +// real-world failure where the model retries a broken tool (e.g. a git command +// that fails because no credentials are configured) indefinitely. +type alwaysFailingToolProvider struct { + mu sync.Mutex + callCount int +} + +func (p *alwaysFailingToolProvider) Chat( + ctx context.Context, + _ []providers.Message, + _ []providers.ToolDefinition, + _ string, + _ map[string]any, +) (*providers.LLMResponse, error) { + p.mu.Lock() + p.callCount++ + p.mu.Unlock() + return &providers.LLMResponse{ + Content: "let me try that again", + ToolCalls: []providers.ToolCall{ + { + ID: "call_1", + Name: "mock_fail", + Arguments: map[string]any{ + "action": "git", + }, + }, + }, + FinishReason: "tool_calls", + }, nil +} + +func (p *alwaysFailingToolProvider) GetDefaultModel() string { + return "always-failing-model" +} + +func (p *alwaysFailingToolProvider) callCountSnapshot() int { + p.mu.Lock() + defer p.mu.Unlock() + return p.callCount +} + +// alwaysFailTool is a tool that always returns the same error, exactly like +// the shell safety guard returning "Command blocked by safety guard" or git +// failing with "could not read Username". +type alwaysFailTool struct { + mu sync.Mutex + execCount int +} + +func (m *alwaysFailTool) Name() string { return "mock_fail" } + +func (m *alwaysFailTool) Description() string { + return "Always fails for testing the repeated-failure loop" +} + +func (m *alwaysFailTool) Parameters() map[string]any { + return map[string]any{ + "type": "object", + "properties": map[string]any{}, + "additionalProperties": true, + } +} + +func (m *alwaysFailTool) Execute(_ context.Context, _ map[string]any) *tools.ToolResult { + m.mu.Lock() + m.execCount++ + m.mu.Unlock() + return tools.ErrorResult("Command blocked by safety guard (dangerous pattern detected)") +} + +func (m *alwaysFailTool) execCountSnapshot() int { + m.mu.Lock() + defer m.mu.Unlock() + return m.execCount +} + +// TestRepeatedIdenticalToolFailure_LoopsToMaxWithoutUserFeedback reproduces a +// real bug observed in production (issue: telegram messages that never get an +// answer). When a tool fails with the same error on every call, the agent loop +// keeps calling the LLM and re-executing the same broken tool until +// max_tool_iterations, with zero feedback to the user in between. +// +// Desired behavior: detect repeated identical failures and stop the turn early +// with a user-visible explanation instead of spinning silently to the cap. +func TestRepeatedIdenticalToolFailure_LoopsToMaxWithoutUserFeedback(t *testing.T) { + tmpDir := t.TempDir() + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: tmpDir, + ModelName: "test-model", + MaxTokens: 4096, + MaxToolIterations: 5, + }, + }, + } + + msgBus := bus.NewMessageBus() + provider := &alwaysFailingToolProvider{} + failingTool := &alwaysFailTool{} + + al := NewAgentLoop(cfg, msgBus, provider) + al.RegisterTool(failingTool) + defaultAgent := al.registry.GetDefaultAgent() + if defaultAgent == nil { + t.Fatal("expected default agent") + } + + response, err := al.runAgentLoop(context.Background(), defaultAgent, processOptions{ + SessionKey: "session-repeat-fail", + Channel: "telegram", + ChatID: "direct", + UserMessage: "run a git command", + DefaultResponse: defaultResponse, + EnableSummary: false, + SendResponse: false, + InboundContext: &bus.InboundContext{ + Channel: "telegram", + ChatID: "direct", + ChatType: "direct", + SenderID: "tester", + }, + RouteResult: &routing.ResolvedRoute{ + AgentID: "main", + Channel: "telegram", + AccountID: routing.DefaultAccountID, + SessionPolicy: routing.SessionPolicy{ + Dimensions: []string{"sender"}, + }, + MatchedBy: "default", + }, + SessionScope: &session.SessionScope{ + Version: session.ScopeVersionV1, + AgentID: "main", + Channel: "telegram", + Account: routing.DefaultAccountID, + Dimensions: []string{"sender"}, + Values: map[string]string{ + "sender": "tester", + }, + }, + }) + if err != nil { + t.Fatalf("runAgentLoop failed: %v", err) + } + + llmCalls := provider.callCountSnapshot() + toolRuns := failingTool.execCountSnapshot() + + // The loop should not spin all the way to max_tool_iterations when the + // tool fails identically every time. Currently it does (bug). + if toolRuns >= 5 { + t.Errorf("BUG: repeated identical tool failure looped to max_tool_iterations: tool executed %d times (cap=5), no early stop", toolRuns) + } + + // The user should receive an explanation mentioning the actual failure, + // not the generic max-iteration message. + if response == toolLimitResponse { + t.Errorf("BUG: user got generic max_tool_iterations message instead of a message explaining the repeated failure: %q", response) + } + + t.Logf("llm calls=%d, tool executions=%d, final response=%q", llmCalls, toolRuns, response) +} diff --git a/pkg/agent/turn_coord.go b/pkg/agent/turn_coord.go index 166ca95525..6f2e698ff0 100644 --- a/pkg/agent/turn_coord.go +++ b/pkg/agent/turn_coord.go @@ -232,6 +232,27 @@ func (al *AgentLoop) runTurn(ctx context.Context, ts *turnState, pipeline *Pipel // Re-read exec.messages since ExecuteTools may have updated it // (added tool results/skipped messages) before returning ControlContinue messages = exec.messages + + // Circuit breaker: if the same tool failed with the same error + // repeatedly, stop early with a helpful message instead of + // spinning silently to max_tool_iterations. + if count, tool, errorSummary := ts.repeatedFailureSnapshot(); count >= repeatedFailureThreshold { + logger.InfoCF("agent", "Stopping turn: repeated identical tool failure", + map[string]any{ + "agent_id": ts.agentID, + "turn_id": ts.turnID, + "iteration": iteration, + "tool": tool, + "failures": count, + "error": errorSummary, + }) + finalContent = fmt.Sprintf(repeatedFailureResponse, tool, count, errorSummary) + result, finalizeErr := pipeline.Finalize(ctx, turnCtx, ts, exec, turnStatus, finalContent) + if finalizeErr != nil { + turnStatus = TurnEndStatusError + } + return result, finalizeErr + } continue case ToolControlBreak: // Hard abort: delegate to abortTurn (sets TurnEndStatusAborted) diff --git a/pkg/agent/turn_state.go b/pkg/agent/turn_state.go index 2440b5a4ce..0135ee0035 100644 --- a/pkg/agent/turn_state.go +++ b/pkg/agent/turn_state.go @@ -197,6 +197,13 @@ type turnState struct { toolExecutions []ToolExecutionRecord turnCtx *TurnContext + // Repeated identical failure tracking (circuit breaker). When the same + // tool fails with the same error N consecutive times, the loop stops early + // instead of spinning to max_tool_iterations with no user feedback. + lastFailureTool string + lastFailureError string + consecutiveFailures int + channel string chatID string workspace string @@ -453,6 +460,31 @@ func (ts *turnState) recordToolExecution(tool string, success bool, errorSummary ErrorSummary: strings.TrimSpace(errorSummary), SkillNames: append([]string(nil), skillNames...), }) + + // Track consecutive identical failures. Any success (or a different + // tool/error) resets the counter. + errorSummary = strings.TrimSpace(errorSummary) + if !success && errorSummary != "" { + if tool == ts.lastFailureTool && errorSummary == ts.lastFailureError { + ts.consecutiveFailures++ + } else { + ts.lastFailureTool = tool + ts.lastFailureError = errorSummary + ts.consecutiveFailures = 1 + } + } else { + ts.lastFailureTool = "" + ts.lastFailureError = "" + ts.consecutiveFailures = 0 + } +} + +// repeatedFailureSnapshot returns the current consecutive identical failure +// count along with the tool name and error that produced it. +func (ts *turnState) repeatedFailureSnapshot() (count int, tool string, errorSummary string) { + ts.mu.RLock() + defer ts.mu.RUnlock() + return ts.consecutiveFailures, ts.lastFailureTool, ts.lastFailureError } func (ts *turnState) toolExecutionsSnapshot() []ToolExecutionRecord {