fix(agent): make same-call repeat guard progress-aware #21

Merged
steve merged 2 commits from fix/progress-aware-same-call-guard into main 2026-07-18 23:22:10 +00:00
2 changed files with 91 additions and 11 deletions
Showing only changes of commit 68bf7157d3 - Show all commits
+43 -11
View File
@@ -125,10 +125,12 @@ func WithCompactor(fn func(ctx context.Context, msgs []llm.Message) ([]llm.Messa
} }
// WithToolErrorLimits installs loop guards: maxConsecutiveErrors bounds // WithToolErrorLimits installs loop guards: maxConsecutiveErrors bounds
// successive steps whose tool results were ALL errors, and // successive steps whose tool results were ALL errors, and maxSameCallRepeats
// maxSameCallRepeats bounds identical (name + arguments) tool calls within // bounds identical (name + arguments) tool calls that ALSO return an unchanged
// one run. Either guard tripping ends the run with ErrToolLoop and the // result within one run — a call whose result keeps advancing (e.g. polling a
// partial result. Zero disables a guard. // long-running background job) is progress and never trips this guard. Either
// guard tripping ends the run with ErrToolLoop and the partial result. Zero
// disables a guard.
func WithToolErrorLimits(maxConsecutiveErrors, maxSameCallRepeats int) Option { func WithToolErrorLimits(maxConsecutiveErrors, maxSameCallRepeats int) Option {
return func(a *Agent) { return func(a *Agent) {
a.maxConsecutiveToolErrors = maxConsecutiveErrors a.maxConsecutiveToolErrors = maxConsecutiveErrors
@@ -276,7 +278,13 @@ func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Resu
// Loop-guard state (WithToolErrorLimits). // Loop-guard state (WithToolErrorLimits).
consecutiveErrorSteps := 0 consecutiveErrorSteps := 0
// callCounts tracks a run-length of identical (name+arguments) tool calls
// that ALSO returned the same result. lastResults holds the previous result
// per signature so a call whose result keeps changing (e.g. polling a
// background job whose progress advances) resets the count instead of
// tripping the guard — a changing result is progress, not a stuck loop.
callCounts := make(map[string]int) callCounts := make(map[string]int)
lastResults := make(map[string]string)
maxSteps := func() int { maxSteps := func() int {
if a.maxStepsFunc != nil { if a.maxStepsFunc != nil {
@@ -330,13 +338,6 @@ func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Resu
result.Messages = msgs result.Messages = msgs
return result, err return result, err
} }
if a.maxSameCallRepeats > 0 {
sig := call.Name + "\x00" + string(call.Arguments)
callCounts[sig]++
if callCounts[sig] > a.maxSameCallRepeats {
repeatTripped = call.Name
}
}
tool, ok := byName[call.Name] tool, ok := byName[call.Name]
if !ok { if !ok {
results = append(results, llm.ToolResult{ results = append(results, llm.ToolResult{
@@ -356,6 +357,37 @@ func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Resu
a.notify(rc, step) a.notify(rc, step)
msgs = append(msgs, llm.ToolResultsMessage(results...)) msgs = append(msgs, llm.ToolResultsMessage(results...))
// Same-call repeat guard (progress-aware). An identical (name+arguments)
// call only counts toward the loop trip when its result is unchanged from
// the previous identical call. A call whose result advances each time —
// canonically polling a long-running background job (elapsed/status keeps
// moving) — resets its count and never trips, while a genuinely stuck
// call returning the same output repeats until it exceeds the ceiling.
// results[i] pairs with resp.ToolCalls[i]: every call appends exactly one
// result (unknown tools append an error result before continue), and the
// only early exit above is a full return on ctx cancellation.
if a.maxSameCallRepeats > 0 {
for i, call := range resp.ToolCalls {
sig := call.Name + "\x00" + string(call.Arguments)
resKey := ""
if i < len(results) {
if results[i].IsError {
resKey = "e\x00"
}
resKey += results[i].Content
}
if prev, seen := lastResults[sig]; seen && prev == resKey {
Review

🟠 Progress check is exact-string equality on the full tool result, so any incidental non-progress field (timestamp, request id, sequence nonce) that changes per-call defeats the guard for genuinely stuck non-erroring polls, which the consecutive-error guard does not cover

correctness, error-handling · flagged by 2 models

Finding 1 (agent/agent.go:379): Confirmed. ExecuteTool (llm/tool.go:190) does a raw json.Marshal of whatever the handler returns with zero normalization, and the guard at agent.go:371-384 does byte-for-byte string comparison of the full encoded Content. A poll response with a jittery field (timestamp, elapsed, request id) alongside an unchanged status will produce a different resKey every call, so callCounts[sig] never accumulates. I confirmed there's no run-critic implementation…

🪰 Gadfly · advisory

🟠 **Progress check is exact-string equality on the full tool result, so any incidental non-progress field (timestamp, request id, sequence nonce) that changes per-call defeats the guard for genuinely stuck non-erroring polls, which the consecutive-error guard does not cover** _correctness, error-handling · flagged by 2 models_ **Finding 1** (agent/agent.go:379): Confirmed. `ExecuteTool` (llm/tool.go:190) does a raw `json.Marshal` of whatever the handler returns with zero normalization, and the guard at agent.go:371-384 does byte-for-byte string comparison of the full encoded `Content`. A poll response with a jittery field (timestamp, elapsed, request id) alongside an unchanged `status` will produce a different `resKey` every call, so `callCounts[sig]` never accumulates. I confirmed there's no run-critic implementation… <sub>🪰 Gadfly · advisory</sub>
callCounts[sig]++
} else {
callCounts[sig] = 1
}
lastResults[sig] = resKey
if callCounts[sig] > a.maxSameCallRepeats {
repeatTripped = call.Name
}
}
}
if repeatTripped != "" { if repeatTripped != "" {
result.Messages = msgs result.Messages = msgs
return result, fmt.Errorf("%w: %q called identically more than %d times", return result, fmt.Errorf("%w: %q called identically more than %d times",
+48
View File
@@ -173,3 +173,51 @@ func TestSameCallRepeatGuard(t *testing.T) {
t.Errorf("varied calls must not trip the guard: %v", err) t.Errorf("varied calls must not trip the guard: %v", err)
} }
} }
// TestSameCallRepeatGuardProgressAware: identical (name+args) calls whose
// RESULT keeps changing — canonically polling a long-running background job
// whose progress advances — do not trip the repeat guard even well past the
// limit; but identical calls returning an unchanged result still trip it.
func TestSameCallRepeatGuardProgressAware(t *testing.T) {
Review

🟡 New progress-aware test duplicates poll-tool/agent scaffolding; a shared helper (cf. adderToolbox) would reduce copy-paste

maintainability · flagged by 1 model

  • agent/hooks_test.go:181-223 — duplicated fixture setup. TestSameCallRepeatGuardProgressAware rebuilds polling/fp and frozen/fp2 with near-identical scaffolding (toolCallReply("c", "poll", ...) + WithToolErrorLimits(0, 3) + WithMaxSteps(20)). A small table-driven helper or shared newPollAgent(t, handler) would cut ~15 lines of copy-paste and match the style of the existing adderToolbox(t) helper used throughout this file. Small; not blocking.

🪰 Gadfly · advisory

🟡 **New progress-aware test duplicates poll-tool/agent scaffolding; a shared helper (cf. adderToolbox) would reduce copy-paste** _maintainability · flagged by 1 model_ - **`agent/hooks_test.go:181-223` — duplicated fixture setup.** `TestSameCallRepeatGuardProgressAware` rebuilds `polling`/`fp` and `frozen`/`fp2` with near-identical scaffolding (`toolCallReply("c", "poll", ...)` + `WithToolErrorLimits(0, 3)` + `WithMaxSteps(20)`). A small table-driven helper or shared `newPollAgent(t, handler)` would cut ~15 lines of copy-paste and match the style of the existing `adderToolbox(t)` helper used throughout this file. Small; not blocking. <sub>🪰 Gadfly · advisory</sub>
// A poll tool that advances every call, so identical args yield a
// different result each time.
calls := 0
polling := llm.NewToolbox("jobs", llm.Tool{
Name: "poll",
Handler: func(context.Context, json.RawMessage) (any, error) {
calls++
return map[string]any{"status": "running", "elapsed": calls}, nil
},
})
n := 0
fp := fake.New("fp", fake.WithDefault(func(string, llm.Request) fake.Step {
n++
if n > 6 { // six identical polls, well past the limit of 3
return fake.Reply("done")
}
return toolCallReply("c", "poll", `{"job":"x"}`)
}))
a := New(newModel(t, fp), "", WithToolbox(polling), WithToolErrorLimits(0, 3), WithMaxSteps(20))
res, err := a.Run(context.Background(), "go")
if err != nil {
t.Fatalf("advancing-result polls must not trip the guard: %v", err)
}
if res.Output != "done" {
t.Errorf("output = %q, want run to complete after polling", res.Output)
}
// A call that returns an UNCHANGED result each time still trips the guard.
frozen := llm.NewToolbox("jobs", llm.Tool{
Name: "poll",
Handler: func(context.Context, json.RawMessage) (any, error) {
return map[string]any{"status": "running"}, nil // never advances
},
})
fp2 := fake.New("fp", fake.WithDefault(func(string, llm.Request) fake.Step {
return toolCallReply("c", "poll", `{"job":"x"}`)
}))
a2 := New(newModel(t, fp2), "", WithToolbox(frozen), WithToolErrorLimits(0, 3), WithMaxSteps(20))
if _, err := a2.Run(context.Background(), "go"); !errors.Is(err, ErrToolLoop) {
t.Fatalf("frozen identical result must still trip the guard: %v", err)
}
}