Skip to content

Commit 8afc6a4

Browse files
committed
revert(hooks): drop loop_detector builtin, keep inline tool-loop detector
Reverts the loop-detector portion of the builtin extraction. The behavior change from per-batch to per-call detection turned out to be the wrong trade-off: alternating multi-tool batches like `[A,B] [A,B]` are a real failure mode that the inline detector caught and the per-call builtin missed. Surgical revert \u2014 keeps the rest of the work intact: * post_tool_use / before_llm_call deny contract widening stays (general feature, no behavior change without an explicit hook). * max_iterations builtin stays (separate from loop detection, no change to the existing agent.MaxIterations UX). * The five context-injecting builtins stay. * The duplicate-dispatch fix and simplifications stay. ## Removed * pkg/hooks/builtins/loop_detector.go and its test. * pkg/hooks/builtins/json.go (canonicalToolInput / sortKeys were only consumed by the loop_detector builtin). * `builtins.LoopDetector` constant. * `AgentDefaults.MaxConsecutiveToolCalls` and `ExemptLoopTools` fields, plus the auto-injection branch in ApplyAgentDefaults. * `State.loopDetector` field. ## Restored from origin/main * pkg/runtime/tool_loop_detector.go (per-batch signature detector). * pkg/runtime/tool_loop_detector_test.go. * Inline `loopDetector := newToolLoopDetector(...)` setup at the top of RunStream's goroutine. * `if loopDetector.record(res.Calls)` block emitting Error + notifyError after a tool batch in loop.go. * `MaxConsecutiveToolCalls` is once again read straight from the session, with the historic threshold-of-5 default. ## Tests * hooks_wiring_test.go: "no flags: no implicit hooks, no executor" case is back; loop_detector-specific assertions removed. * builtins_test.go: removed LoopDetector from the registration roster. ## Validation go build ./... -> ok go test ./... -> all packages pass go test -race ./pkg/hooks/... ./pkg/runtime/... -> clean golangci-lint run ./... -> 0 issues Assisted-By: docker-agent
1 parent c54be36 commit 8afc6a4

10 files changed

Lines changed: 392 additions & 496 deletions

File tree

pkg/hooks/builtins/builtins.go

Lines changed: 9 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Package builtins contains the in-process hook implementations
1+
// Package builtins contains the stock in-process hook implementations
22
// shipped with docker-agent.
33
//
44
// Available builtins:
@@ -11,27 +11,22 @@
1111
// - add_directory_listing (session_start) — top-level entries of cwd
1212
// - add_user_info (session_start) — current OS user and host
1313
// - add_recent_commits (session_start) — `git log --oneline -n N`
14-
// - loop_detector (post_tool_use) — block on N consecutive
15-
// identical tool calls
1614
// - max_iterations (before_llm_call) — hard stop after N model calls
1715
//
1816
// Reference any of them from a hook YAML entry as
1917
// `{type: builtin, command: "<name>"}`. The runtime additionally
2018
// auto-injects add_date / add_environment_info / add_prompt_files
21-
// from the matching agent flags, and loop_detector from
22-
// agent.MaxConsecutiveToolCalls.
19+
// from the matching agent flags.
2320
//
2421
// turn_start builtins recompute every turn (date, git state).
2522
// session_start builtins run once per session for context that's
26-
// stable for its duration. loop_detector and max_iterations are
27-
// stateful: their per-session counters live on the [State] returned
28-
// by [Register]; the runtime clears them via [State.ClearSession]
29-
// from session_end.
23+
// stable for its duration. max_iterations is stateful: its
24+
// per-session counter lives on the [State] returned by [Register];
25+
// the runtime clears it via [State.ClearSession] from session_end.
3026
package builtins
3127

3228
import (
3329
"errors"
34-
"strconv"
3530

3631
"github.com/docker/docker-agent/pkg/hooks"
3732
)
@@ -40,7 +35,6 @@ import (
4035
// It is returned by [Register] so callers can clear per-session
4136
// entries on session_end. Stateless builtins don't appear here.
4237
type State struct {
43-
loopDetector *loopDetectorBuiltin
4438
maxIterations *maxIterationsBuiltin
4539
}
4640

@@ -50,15 +44,13 @@ func (s *State) ClearSession(sessionID string) {
5044
if s == nil || sessionID == "" {
5145
return
5246
}
53-
s.loopDetector.clearSession(sessionID)
5447
s.maxIterations.clearSession(sessionID)
5548
}
5649

5750
// Register installs the stock builtin hooks on r and returns a [State]
5851
// handle the caller must use to clear per-session state on session_end.
5952
func Register(r *hooks.Registry) (*State, error) {
6053
state := &State{
61-
loopDetector: newLoopDetector(),
6254
maxIterations: newMaxIterations(),
6355
}
6456
if err := errors.Join(
@@ -70,7 +62,6 @@ func Register(r *hooks.Registry) (*State, error) {
7062
r.RegisterBuiltin(AddDirectoryListing, addDirectoryListing),
7163
r.RegisterBuiltin(AddUserInfo, addUserInfo),
7264
r.RegisterBuiltin(AddRecentCommits, addRecentCommits),
73-
r.RegisterBuiltin(LoopDetector, state.loopDetector.hook),
7465
r.RegisterBuiltin(MaxIterations, state.maxIterations.hook),
7566
); err != nil {
7667
return nil, err
@@ -79,19 +70,11 @@ func Register(r *hooks.Registry) (*State, error) {
7970
}
8071

8172
// AgentDefaults captures the agent-level flags that map onto stock
82-
// builtin hook entries.
83-
//
84-
// MaxConsecutiveToolCalls and ExemptLoopTools together control the
85-
// auto-injected loop_detector entry: a non-zero threshold injects a
86-
// post_tool_use hook with that threshold and the exempt tool names.
87-
// The runtime supplies the exempt list so this package stays
88-
// decoupled from any tool constants.
73+
// builtin hook entries. Pass each AgentConfig.AddXxx flag as-is.
8974
type AgentDefaults struct {
90-
AddDate bool
91-
AddEnvironmentInfo bool
92-
AddPromptFiles []string
93-
MaxConsecutiveToolCalls int
94-
ExemptLoopTools []string
75+
AddDate bool
76+
AddEnvironmentInfo bool
77+
AddPromptFiles []string
9578
}
9679

9780
// ApplyAgentDefaults appends the stock builtin hook entries implied by
@@ -110,13 +93,6 @@ func ApplyAgentDefaults(cfg *hooks.Config, d AgentDefaults) *hooks.Config {
11093
if d.AddEnvironmentInfo {
11194
cfg.SessionStart = append(cfg.SessionStart, builtinHook(AddEnvironmentInfo))
11295
}
113-
if d.MaxConsecutiveToolCalls > 0 {
114-
args := append([]string{strconv.Itoa(d.MaxConsecutiveToolCalls)}, d.ExemptLoopTools...)
115-
cfg.PostToolUse = append(cfg.PostToolUse, hooks.MatcherConfig{
116-
Matcher: "*",
117-
Hooks: []hooks.Hook{builtinHook(LoopDetector, args...)},
118-
})
119-
}
12096
if cfg.IsEmpty() {
12197
return nil
12298
}

pkg/hooks/builtins/builtins_test.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ func TestRegisterInstallsAllBuiltins(t *testing.T) {
3333
builtins.AddDirectoryListing,
3434
builtins.AddUserInfo,
3535
builtins.AddRecentCommits,
36-
builtins.LoopDetector,
3736
builtins.MaxIterations,
3837
} {
3938
fn, ok := r.LookupBuiltin(name)

pkg/hooks/builtins/json.go

Lines changed: 0 additions & 47 deletions
This file was deleted.

pkg/hooks/builtins/loop_detector.go

Lines changed: 0 additions & 109 deletions
This file was deleted.

0 commit comments

Comments
 (0)