fix(agent): make same-call repeat guard progress-aware #21
@@ -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
|
||||||
@@ -251,6 +253,15 @@ func (a *Agent) mergedTools() (map[string]llm.Tool, []llm.Tool, error) {
|
|||||||
// Run executes the loop: send the conversation; while the model requests
|
// Run executes the loop: send the conversation; while the model requests
|
||||||
// tools, execute them and feed results back; stop on a final answer,
|
// tools, execute them and feed results back; stop on a final answer,
|
||||||
// MaxSteps, or an unrecoverable model error.
|
// MaxSteps, or an unrecoverable model error.
|
||||||
|
// repeatState is the per-signature bookkeeping for the progress-aware same-call
|
||||||
|
// guard: count is the run-length of consecutive identical calls that returned
|
||||||
|
// lastResult (the previous call's encoded result). A changed result resets count
|
||||||
|
// to 1. See the guard block in Run.
|
||||||
|
type repeatState struct {
|
||||||
|
count int
|
||||||
|
lastResult string
|
||||||
|
}
|
||||||
|
|
||||||
func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Result, error) {
|
func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Result, error) {
|
||||||
var rc runConfig
|
var rc runConfig
|
||||||
for _, opt := range opts {
|
for _, opt := range opts {
|
||||||
@@ -274,9 +285,12 @@ func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Resu
|
|||||||
reqOpts := append(append([]llm.Option(nil), a.reqOpts...), rc.reqOpts...)
|
reqOpts := append(append([]llm.Option(nil), a.reqOpts...), rc.reqOpts...)
|
||||||
system := a.systemPrompt()
|
system := a.systemPrompt()
|
||||||
|
|
||||||
|
|
|||||||
// Loop-guard state (WithToolErrorLimits).
|
// Loop-guard state (WithToolErrorLimits). repeatStates tracks, per identical
|
||||||
|
// (name+arguments) signature, the run-length of consecutive calls that
|
||||||
|
// returned the same result and that last result — see the same-call guard
|
||||||
|
// below.
|
||||||
consecutiveErrorSteps := 0
|
consecutiveErrorSteps := 0
|
||||||
callCounts := make(map[string]int)
|
repeatStates := make(map[string]*repeatState)
|
||||||
|
|
||||||
maxSteps := func() int {
|
maxSteps := func() int {
|
||||||
if a.maxStepsFunc != nil {
|
if a.maxStepsFunc != nil {
|
||||||
@@ -330,13 +344,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 +363,42 @@ 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 keeps advancing —
|
||||||
|
// canonically polling a long-running background job — is progress and
|
||||||
|
// resets its count, while a genuinely stuck call returning the same output
|
||||||
|
// trips once it exceeds the ceiling. Result equality is exact-string on
|
||||||
|
// the full encoded content, chosen to err toward NOT tripping: a hung job
|
||||||
|
// whose poll still reports a ticking field is left to MaxRuntime / the
|
||||||
|
gitea-actions
commented
🟡 Defensive error-handling, maintainability · flagged by 4 models
🪰 Gadfly · advisory 🟡 **Defensive `i < len(results)` branch is dead code that contradicts the invariant the preceding comment asserts**
_error-handling, maintainability · flagged by 4 models_
- `agent/agent.go:373` — the `if i < len(results) { … }` branch is defensive dead code that contradicts the invariant the preceding comment (lines 366-368) asserts. Verified against the actual loop above: the `ctx.Err()` path does a full `return` before reaching the guard (lines 337-340), the unknown-tool path appends an error result before `continue` (lines 342-348), and `ExecuteTool` (`llm/tool.go:156`) always returns exactly one `ToolResult` (panic recovered into an error result, nil handler…
<sub>🪰 Gadfly · advisory</sub>
|
|||||||
|
// job's own ceiling rather than risking a false kill of real progress.
|
||||||
|
// 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 {
|
||||||
|
gitea-actions
commented
🟠 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. 🪰 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>
|
|||||||
|
sig := call.Name + "\x00" + string(call.Arguments)
|
||||||
|
resKey := results[i].Content
|
||||||
|
if results[i].IsError {
|
||||||
|
resKey = "e\x00" + resKey
|
||||||
|
}
|
||||||
|
st := repeatStates[sig]
|
||||||
|
if st == nil {
|
||||||
|
st = &repeatState{}
|
||||||
|
repeatStates[sig] = st
|
||||||
|
}
|
||||||
|
if st.count > 0 && st.lastResult == resKey {
|
||||||
|
st.count++
|
||||||
|
} else {
|
||||||
|
st.count = 1
|
||||||
|
}
|
||||||
|
st.lastResult = resKey
|
||||||
|
if st.count > 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",
|
||||||
|
|||||||
@@ -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) {
|
||||||
|
gitea-actions
commented
🟡 New progress-aware test duplicates poll-tool/agent scaffolding; a shared helper (cf. adderToolbox) would reduce copy-paste maintainability · flagged by 1 model
🪰 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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
🟡 callCounts and lastResults are two parallel maps keyed by the same signature that should be merged
maintainability, performance · flagged by 2 models
agent/agent.go:286-287/369-389—callCountsandlastResultsare two separate maps keyed by the samesig, always read and written together in lockstep (agent.go:379-384). This is the classic "two maps that must stay in sync" smell; merging them into onemap[string]struct{ count int; lastResult string }would remove the duplicate lookup per call and make the pairing structurally guaranteed rather than convention-guaranteed. Verified by reading the full guard loop — every access t…🪰 Gadfly · advisory