steve/majordomo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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
Conversation 3 Commits 2 Files Changed 2
+36 -25
1 changed files with 36 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 9922166d7a - Show all commits
agent/agent.go
+36 -25
View File
@@ -253,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
// tools, execute them and feed results back; stop on a final answer,
// 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) {
var rc runConfig
for _, opt := range opts {
@@ -276,15 +285,12 @@ func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Resu
reqOpts := append(append([]llm.Option(nil), a.reqOpts...), rc.reqOpts...)
system := a.systemPrompt()
gitea-actions commented 2026-07-18 23:07:09 +00:00
Outdated
Review
Copy Link
Copy Source

🟡 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 — callCounts and lastResults are two separate maps keyed by the same sig, 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 one map[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

🟡 **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` — `callCounts` and `lastResults` are two separate maps keyed by the same `sig`, 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 one `map[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… <sub>🪰 Gadfly · advisory</sub>
// 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
// 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)
lastResults := make(map[string]string)
repeatStates := make(map[string]*repeatState)
maxSteps := func() int {
if a.maxStepsFunc != nil {
1
 @@ -359,30 +365,35 @@ func (a *Agent) Run(ctx context.Context, input string, opts ...RunOption) (*Resu
// 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
// 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 2026-07-18 23:07:09 +00:00
Outdated
Review
Copy Link
Copy Source

🟡 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…

🪰 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 2026-07-18 23:07:09 +00:00
Review
Copy Link
Copy Source

🟠 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>
sig := call.Name + "\x00" + string(call.Arguments)
resKey := ""
if i < len(results) {
if results[i].IsError {
resKey = "e\x00"
}
resKey += results[i].Content
resKey := results[i].Content
if results[i].IsError {
resKey = "e\x00" + resKey
}
if prev, seen := lastResults[sig]; seen && prev == resKey {
callCounts[sig]++
st := repeatStates[sig]
if st == nil {
st = &repeatState{}
repeatStates[sig] = st
}
if st.count > 0 && st.lastResult == resKey {
st.count++
} else {
callCounts[sig] = 1
st.count = 1
}
lastResults[sig] = resKey
if callCounts[sig] > a.maxSameCallRepeats {
st.lastResult = resKey
if st.count > a.maxSameCallRepeats {
repeatTripped = call.Name
}
}