From 42263e5cc5f87c1410635bbda60f683ddd5c9b75 Mon Sep 17 00:00:00 2001 From: viettranx Date: Fri, 27 Feb 2026 22:00:50 +0700 Subject: [PATCH] feat: Add tool loop detection, negative context injection, and fix Docker workspace permissions - Add tool loop detection (toolloop.go): tracks repeated no-progress tool calls using SHA256 hashing of args+results. Warning at 5 identical calls, force stop at 10. Prevents Gemini models from burning tokens in infinite loops. - Inject AVAILABILITY.md negative context when agent has no team/delegation targets, so models don't waste iterations probing unavailable capabilities. - Fix Dockerfile: create /app/.goclaw directory so Docker volume initializes with correct goclaw:goclaw ownership instead of root:root. - Update collapseToolCallsWithoutSig comments for clarity. Co-Authored-By: Claude Opus 4.6 --- Dockerfile | 2 +- internal/agent/loop.go | 38 +++++++ internal/agent/resolver.go | 25 +++++ internal/agent/systemprompt_sections.go | 8 +- internal/agent/toolloop.go | 137 ++++++++++++++++++++++++ internal/agent/toolloop_test.go | 117 ++++++++++++++++++++ internal/providers/openai.go | 7 +- internal/providers/openai_gemini.go | 4 +- internal/providers/types.go | 6 +- 9 files changed, 331 insertions(+), 13 deletions(-) create mode 100644 internal/agent/toolloop.go create mode 100644 internal/agent/toolloop_test.go diff --git a/Dockerfile b/Dockerfile index 5b084f55..c2d6a5f1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -52,7 +52,7 @@ COPY docker-entrypoint.sh /app/docker-entrypoint.sh RUN chmod +x /app/docker-entrypoint.sh # Create data directories (owned by goclaw user) -RUN mkdir -p /app/workspace /app/data /app/sessions /app/skills /app/tsnet-state \ +RUN mkdir -p /app/workspace /app/data /app/sessions /app/skills /app/tsnet-state /app/.goclaw \ && chown -R goclaw:goclaw /app # Default environment diff --git a/internal/agent/loop.go b/internal/agent/loop.go index 5133f87c..60df5d0d 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -494,6 +494,7 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) }) // 4. Run LLM iteration loop + var loopDetector toolLoopState // detects repeated no-progress tool calls var totalUsage providers.Usage iteration := 0 var finalContent string @@ -600,11 +601,16 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) argsJSON, _ := json.Marshal(tc.Arguments) slog.Info("tool call", "agent", l.id, "tool", tc.Name, "args_len", len(argsJSON)) + argsHash := loopDetector.record(tc.Name, tc.Arguments) + toolSpanStart := time.Now().UTC() result := l.tools.ExecuteWithContext(ctx, tc.Name, tc.Arguments, req.Channel, req.ChatID, req.PeerKind, req.SessionKey, nil) l.emitToolSpan(ctx, toolSpanStart, tc.Name, tc.ID, string(argsJSON), result) + // Record result for loop detection. + loopDetector.recordResult(argsHash, result.ForLLM) + if result.Async { asyncToolCalls = append(asyncToolCalls, tc.Name) } @@ -640,6 +646,18 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) } messages = append(messages, toolMsg) pendingMsgs = append(pendingMsgs, toolMsg) + + // Check for tool call loop after recording result. + if level, msg := loopDetector.detect(tc.Name, argsHash); level != "" { + if level == "critical" { + slog.Warn("tool loop critical", "agent", l.id, "tool", tc.Name, "message", msg) + finalContent = "I was unable to complete this task — I got stuck repeatedly calling " + tc.Name + " without making progress. Please try rephrasing your request." + break + } + // Warning: inject message so model knows to change strategy. + slog.Warn("tool loop warning", "agent", l.id, "tool", tc.Name, "message", msg) + messages = append(messages, providers.Message{Role: "user", Content: msg}) + } } else { // Multiple tools: parallel execution via goroutines. // Tool instances are immutable (context-based) so concurrent access is safe. @@ -693,9 +711,14 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) }) // 5. Process results sequentially: emit events, append messages, save to session + var loopStuck bool for _, r := range collected { l.emitToolSpan(ctx, r.spanStart, r.tc.Name, r.tc.ID, r.argsJSON, r.result) + // Record for loop detection. + argsHash := loopDetector.record(r.tc.Name, r.tc.Arguments) + loopDetector.recordResult(argsHash, r.result.ForLLM) + if r.result.Async { asyncToolCalls = append(asyncToolCalls, r.tc.Name) } @@ -731,6 +754,21 @@ func (l *Loop) runLoop(ctx context.Context, req RunRequest) (*RunResult, error) } messages = append(messages, toolMsg) pendingMsgs = append(pendingMsgs, toolMsg) + + // Check for tool call loop. + if level, msg := loopDetector.detect(r.tc.Name, argsHash); level != "" { + if level == "critical" { + slog.Warn("tool loop critical", "agent", l.id, "tool", r.tc.Name, "message", msg) + finalContent = "I was unable to complete this task — I got stuck repeatedly calling " + r.tc.Name + " without making progress. Please try rephrasing your request." + loopStuck = true + break + } + slog.Warn("tool loop warning", "agent", l.id, "tool", r.tc.Name, "message", msg) + messages = append(messages, providers.Message{Role: "user", Content: msg}) + } + } + if loopStuck { + break } } } diff --git a/internal/agent/resolver.go b/internal/agent/resolver.go index 030e8331..8b715660 100644 --- a/internal/agent/resolver.go +++ b/internal/agent/resolver.go @@ -102,6 +102,7 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { // Inject DELEGATION.md from delegation links (only if not already present in DB). // Uses DELEGATION.md (not AGENTS.md) to avoid collision with per-user AGENTS.md // which contains workspace instructions for open agents. + hasDelegation := false if deps.AgentLinkStore != nil { hasDelegationMD := false for _, cf := range contextFiles { @@ -118,23 +119,28 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { targets := filterManualLinks(allTargets) if len(targets) > 0 && len(targets) <= 15 { // Static list: all targets directly + hasDelegation = true contextFiles = append(contextFiles, bootstrap.ContextFile{ Path: bootstrap.DelegationFile, Content: buildDelegateAgentsMD(targets), }) } else if len(targets) > 15 { // Too many targets: instruct agent to use delegate_search tool + hasDelegation = true contextFiles = append(contextFiles, bootstrap.ContextFile{ Path: bootstrap.DelegationFile, Content: buildDelegateSearchInstruction(len(targets)), }) } } + } else { + hasDelegation = true } } // Inject TEAM.md for all team members (lead + members) so every agent // knows the team workflow: create/claim/complete tasks via team_tasks tool. + hasTeam := false if deps.TeamStore != nil { hasTeamMD := false for _, cf := range contextFiles { @@ -146,15 +152,34 @@ func NewManagedResolver(deps ResolverDeps) ResolverFunc { if !hasTeamMD { if team, err := deps.TeamStore.GetTeamForAgent(ctx, ag.ID); err == nil && team != nil { if members, err := deps.TeamStore.ListMembers(ctx, team.ID); err == nil { + hasTeam = true contextFiles = append(contextFiles, bootstrap.ContextFile{ Path: bootstrap.TeamFile, Content: buildTeamMD(team, members, ag.ID), }) } } + } else { + hasTeam = true } } + // Inject negative context so the model doesn't waste iterations probing + // unavailable capabilities (team_tasks, delegate_search, etc.). + if !hasTeam || !hasDelegation { + var notes []string + if !hasTeam { + notes = append(notes, "You are NOT part of any team. Do not use team_tasks or team_message tools.") + } + if !hasDelegation { + notes = append(notes, "You have NO delegation targets. Do not use delegate or delegate_search tools.") + } + contextFiles = append(contextFiles, bootstrap.ContextFile{ + Path: "AVAILABILITY.md", + Content: strings.Join(notes, "\n"), + }) + } + contextWindow := ag.ContextWindow if contextWindow <= 0 { contextWindow = 200000 diff --git a/internal/agent/systemprompt_sections.go b/internal/agent/systemprompt_sections.go index f9942e8a..92d4a11a 100644 --- a/internal/agent/systemprompt_sections.go +++ b/internal/agent/systemprompt_sections.go @@ -104,15 +104,15 @@ func buildProjectContextSection(files []bootstrap.ContextFile) []string { for _, f := range files { base := filepath.Base(f.Path) - // During bootstrap (first run), skip delegation/team files — they add noise + // During bootstrap (first run), skip delegation/team/availability files — they add noise // and waste tokens when the agent should only be introducing itself. - if hasBootstrap && (base == bootstrap.DelegationFile || base == bootstrap.TeamFile) { + if hasBootstrap && (base == bootstrap.DelegationFile || base == bootstrap.TeamFile || base == "AVAILABILITY.md") { continue } - // Virtual files (DELEGATION.md, TEAM.md) are system-injected, not on disk. + // Virtual files (DELEGATION.md, TEAM.md, AVAILABILITY.md) are system-injected, not on disk. // Render with so the LLM doesn't try to read/write them as files. - if base == bootstrap.DelegationFile || base == bootstrap.TeamFile { + if base == bootstrap.DelegationFile || base == bootstrap.TeamFile || base == "AVAILABILITY.md" { lines = append(lines, fmt.Sprintf("", base), f.Content, diff --git a/internal/agent/toolloop.go b/internal/agent/toolloop.go new file mode 100644 index 00000000..d6c863de --- /dev/null +++ b/internal/agent/toolloop.go @@ -0,0 +1,137 @@ +package agent + +import ( + "crypto/sha256" + "encoding/json" + "fmt" + "sort" + "strings" +) + +// Tool loop detection thresholds (per-run, not per-session). +const ( + toolLoopHistorySize = 30 + toolLoopWarningThreshold = 5 // inject warning into conversation + toolLoopCriticalThreshold = 10 // force stop the iteration loop +) + +// toolLoopState tracks recent tool calls within a single agent run +// to detect infinite loops (same tool + same args + same result). +type toolLoopState struct { + history []toolCallRecord +} + +type toolCallRecord struct { + toolName string + argsHash string + resultHash string // empty until result is recorded +} + +// record adds a tool call to history and returns its argsHash. +func (s *toolLoopState) record(toolName string, args map[string]interface{}) string { + h := hashToolCall(toolName, args) + s.history = append(s.history, toolCallRecord{ + toolName: toolName, + argsHash: h, + }) + if len(s.history) > toolLoopHistorySize { + s.history = s.history[len(s.history)-toolLoopHistorySize:] + } + return h +} + +// recordResult updates the most recent matching record with the result hash. +func (s *toolLoopState) recordResult(argsHash, resultContent string) { + rh := hashResult(resultContent) + // Walk backward to find the latest record with matching argsHash and no result yet. + for i := len(s.history) - 1; i >= 0; i-- { + rec := &s.history[i] + if rec.argsHash == argsHash && rec.resultHash == "" { + rec.resultHash = rh + return + } + } +} + +// detect checks for repeated no-progress tool calls. +// Returns level ("warning", "critical", or "") and a human-readable message. +func (s *toolLoopState) detect(toolName string, argsHash string) (level, message string) { + if len(s.history) < toolLoopWarningThreshold { + return "", "" + } + + // Count records with identical argsHash AND identical non-empty resultHash. + // This ensures we only flag true no-progress loops (same input → same output). + var noProgressCount int + var lastResultHash string + + for i := len(s.history) - 1; i >= 0; i-- { + rec := s.history[i] + if rec.argsHash != argsHash { + continue + } + if rec.resultHash == "" { + continue // incomplete record, skip + } + if lastResultHash == "" { + lastResultHash = rec.resultHash + } + if rec.resultHash == lastResultHash { + noProgressCount++ + } + } + + if noProgressCount >= toolLoopCriticalThreshold { + return "critical", fmt.Sprintf( + "CRITICAL: %s has been called %d times with identical arguments and results. "+ + "Stopping to prevent runaway loop.", toolName, noProgressCount) + } + + if noProgressCount >= toolLoopWarningThreshold { + return "warning", fmt.Sprintf( + "[System: WARNING — %s has been called %d times with the same arguments and identical results. "+ + "This is not making progress. Try a completely different approach, use different tools, "+ + "or respond directly to the user with what you know.]", toolName, noProgressCount) + } + + return "", "" +} + +// hashToolCall produces a deterministic hash of tool name + arguments. +func hashToolCall(toolName string, args map[string]interface{}) string { + s := toolName + ":" + stableJSON(args) + h := sha256.Sum256([]byte(s)) + return fmt.Sprintf("%x", h[:16]) // 32 hex chars, enough for dedup +} + +// hashResult produces a hash of tool result content. +func hashResult(content string) string { + h := sha256.Sum256([]byte(content)) + return fmt.Sprintf("%x", h[:16]) +} + +// stableJSON serializes a value with sorted keys for deterministic hashing. +func stableJSON(v interface{}) string { + switch val := v.(type) { + case map[string]interface{}: + keys := make([]string, 0, len(val)) + for k := range val { + keys = append(keys, k) + } + sort.Strings(keys) + parts := make([]string, len(keys)) + for i, k := range keys { + parts[i] = fmt.Sprintf("%q:%s", k, stableJSON(val[k])) + } + return "{" + strings.Join(parts, ",") + "}" + case []interface{}: + parts := make([]string, len(val)) + for i, elem := range val { + parts[i] = stableJSON(elem) + } + return "[" + strings.Join(parts, ",") + "]" + default: + b, _ := json.Marshal(v) + return string(b) + } +} diff --git a/internal/agent/toolloop_test.go b/internal/agent/toolloop_test.go new file mode 100644 index 00000000..e96528c2 --- /dev/null +++ b/internal/agent/toolloop_test.go @@ -0,0 +1,117 @@ +package agent + +import "testing" + +func TestToolLoopDetection_NoLoop(t *testing.T) { + var s toolLoopState + + // 4 identical calls with same result → below threshold, no detection + for i := 0; i < 4; i++ { + h := s.record("list_files", map[string]interface{}{"path": "."}) + s.recordResult(h, "access denied") + level, _ := s.detect("list_files", h) + if level != "" { + t.Fatalf("iteration %d: expected no detection, got %q", i, level) + } + } +} + +func TestToolLoopDetection_Warning(t *testing.T) { + var s toolLoopState + + var lastLevel string + for i := 0; i < toolLoopWarningThreshold; i++ { + h := s.record("list_files", map[string]interface{}{"path": "."}) + s.recordResult(h, "access denied") + lastLevel, _ = s.detect("list_files", h) + } + if lastLevel != "warning" { + t.Fatalf("expected warning after %d calls, got %q", toolLoopWarningThreshold, lastLevel) + } +} + +func TestToolLoopDetection_Critical(t *testing.T) { + var s toolLoopState + + var lastLevel string + for i := 0; i < toolLoopCriticalThreshold; i++ { + h := s.record("list_files", map[string]interface{}{"path": "."}) + s.recordResult(h, "access denied") + lastLevel, _ = s.detect("list_files", h) + } + if lastLevel != "critical" { + t.Fatalf("expected critical after %d calls, got %q", toolLoopCriticalThreshold, lastLevel) + } +} + +func TestToolLoopDetection_DifferentArgs(t *testing.T) { + var s toolLoopState + + // Same tool but different args each time → no detection + for i := 0; i < 15; i++ { + args := map[string]interface{}{"path": string(rune('a' + i))} + h := s.record("list_files", args) + s.recordResult(h, "access denied") + level, _ := s.detect("list_files", h) + if level != "" { + t.Fatalf("iteration %d: expected no detection for different args, got %q", i, level) + } + } +} + +func TestToolLoopDetection_DifferentResults(t *testing.T) { + var s toolLoopState + + // Same args but different results each time → progress, no detection + for i := 0; i < 15; i++ { + h := s.record("web_fetch", map[string]interface{}{"url": "https://example.com"}) + s.recordResult(h, "result content "+string(rune('a'+i))) + level, _ := s.detect("web_fetch", h) + if level != "" { + t.Fatalf("iteration %d: expected no detection for different results, got %q", i, level) + } + } +} + +func TestToolLoopDetection_MixedTools(t *testing.T) { + var s toolLoopState + + // Alternate between two tools with same result → each tool only hit ~half + for i := 0; i < 15; i++ { + toolName := "list_files" + if i%2 == 1 { + toolName = "read_file" + } + h := s.record(toolName, map[string]interface{}{"path": "."}) + s.recordResult(h, "error") + level, _ := s.detect(toolName, h) + // Each tool is only called ~7-8 times, should at most warn + if level == "critical" { + t.Fatalf("iteration %d: unexpected critical for alternating tools", i) + } + } +} + +func TestStableJSON(t *testing.T) { + // Same keys in different order → same hash + a := stableJSON(map[string]interface{}{"b": 2, "a": 1}) + b := stableJSON(map[string]interface{}{"a": 1, "b": 2}) + if a != b { + t.Fatalf("stableJSON not deterministic: %q != %q", a, b) + } +} + +func TestHashToolCall(t *testing.T) { + // Same input → same hash + h1 := hashToolCall("list_files", map[string]interface{}{"path": "."}) + h2 := hashToolCall("list_files", map[string]interface{}{"path": "."}) + if h1 != h2 { + t.Fatal("hashToolCall not deterministic") + } + + // Different tool → different hash + h3 := hashToolCall("read_file", map[string]interface{}{"path": "."}) + if h1 == h3 { + t.Fatal("different tools should have different hashes") + } +} diff --git a/internal/providers/openai.go b/internal/providers/openai.go index b34fce1b..22b8f8b6 100644 --- a/internal/providers/openai.go +++ b/internal/providers/openai.go @@ -188,9 +188,10 @@ func (p *OpenAIProvider) ChatStream(ctx context.Context, req ChatRequest, onChun } func (p *OpenAIProvider) buildRequestBody(model string, req ChatRequest, stream bool) map[string]interface{} { - // Gemini 2.5+: collapse old tool_call cycles missing thought_signature into plain text. - // Old session messages stored before the thought_signature fix don't have it, - // and Gemini rejects them with HTTP 400. Collapse preserves context as text. + // Gemini 2.5+: collapse tool_call cycles missing thought_signature. + // Gemini requires thought_signature echoed back on every tool_call; models that + // don't return it (e.g. gemini-3-flash) will cause HTTP 400 if sent as-is. + // Tool results are folded into plain user messages to preserve context. inputMessages := req.Messages if strings.Contains(strings.ToLower(p.name), "gemini") { inputMessages = collapseToolCallsWithoutSig(inputMessages) diff --git a/internal/providers/openai_gemini.go b/internal/providers/openai_gemini.go index dcd2ce9c..55678187 100644 --- a/internal/providers/openai_gemini.go +++ b/internal/providers/openai_gemini.go @@ -3,8 +3,8 @@ package providers import "strings" // collapseToolCallsWithoutSig rewrites tool_call cycles that lack thought_signature -// (required by Gemini 2.5+). Old session history stored before the thought_signature -// capture fix doesn't have it, and Gemini rejects those messages with HTTP 400. +// (required by Gemini 2.5+). Gemini requires thought_signature echoed back on every +// tool_call; models that don't return it cause HTTP 400 if sent as-is. // // The assistant's tool_calls are stripped, and the corresponding tool-result messages // are folded into a single user message with the tool output content. This preserves diff --git a/internal/providers/types.go b/internal/providers/types.go index 3447331f..4434be77 100644 --- a/internal/providers/types.go +++ b/internal/providers/types.go @@ -21,9 +21,9 @@ type Provider interface { // ChatRequest contains the input for a Chat/ChatStream call. type ChatRequest struct { - Messages []Message `json:"messages"` - Tools []ToolDefinition `json:"tools,omitempty"` - Model string `json:"model,omitempty"` + Messages []Message `json:"messages"` + Tools []ToolDefinition `json:"tools,omitempty"` + Model string `json:"model,omitempty"` Options map[string]interface{} `json:"options,omitempty"` }