diff --git a/agent-schema.json b/agent-schema.json index d73d6591ee..cef7625b60 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -1336,7 +1336,7 @@ }, "tool_response_transform": { "type": "array", - "description": "Hooks that fire between a tool's execution and the runtime's emission/record of the response, with the rewrite reaching event consumers, the persisted session, the post_tool_use hook input, and the next LLM call. A hook may rewrite the tool's textual output by setting hookSpecificOutput.updated_tool_response \u2014 the symmetric counterpart of pre_tool_use's updated_input, applied to tool RESULTS instead of tool ARGUMENTS. The redact_secrets builtin uses this event for the third leg of the redact_secrets feature; custom rewriters can also truncate excessive output, scrub PII, or normalise tool dialects. Tool-matched, like pre_tool_use / post_tool_use.", + "description": "Hooks that fire between a tool's execution and the runtime's emission/record of the response, with the rewrite reaching event consumers, the persisted session, the post_tool_use hook input, and the next LLM call. A hook may rewrite the tool's textual output by setting hookSpecificOutput.updated_tool_response \u2014 the symmetric counterpart of pre_tool_use's updated_input, applied to tool RESULTS instead of tool ARGUMENTS. The redact_secrets builtin uses this event for the third leg of the redact_secrets feature, and the opt-in elide_repeated_tool_results builtin uses it to replace a read-only tool's output with a short marker when it repeats what the model already saw; custom rewriters can also truncate excessive output, scrub PII, or normalise tool dialects. Tool-matched, like pre_tool_use / post_tool_use.", "items": { "$ref": "#/definitions/HookMatcherConfig" } diff --git a/docs/configuration/hooks/index.md b/docs/configuration/hooks/index.md index 34efe9d2b8..5a47018a41 100644 --- a/docs/configuration/hooks/index.md +++ b/docs/configuration/hooks/index.md @@ -217,6 +217,7 @@ Built-ins are typically zero-config and faster than equivalent shell hooks becau | `snapshot` | `session_start`, `turn_start`, `turn_end`, `pre_tool_use`, `post_tool_use`, `session_end` | _none_ | Records filesystem snapshots in a shadow git repo under the Docker Agent data directory. No-op outside git repos; respects the source repo's ignore rules and skips newly-added files larger than 2 MiB. | | `redact_secrets` | `pre_tool_use`, `before_llm_call`, `tool_response_transform` | _none_ | Scrubs detected secrets (API keys, tokens, private keys, …) out of tool call arguments, outgoing chat content, and tool output. The same builtin handles all three events and dispatches on the event name. Auto-registered on all three events by `redact_secrets: true` on the agent — see [`examples/redact_secrets_hooks.yaml`](https://github.com/docker/docker-agent/blob/main/examples/redact_secrets_hooks.yaml) for the manual wiring. | | `limit_large_tool_results` | `tool_response_transform`, `session_end` | _none_ | **Always-on safety hook** — automatically injected by the runtime, no configuration required. When a tool result from the `filesystem`, `shell`, `mcp`, or `a2a` categories exceeds 2,000 lines or 50 KiB, the full payload is written to a per-session temp file and replaced in the conversation with a notice plus a bounded excerpt (2,000 lines, up to 50 KiB): the tail for most tools, but the head for the built-in filesystem `read_file`, whose notice suggests a follow-up call with `line`/`limit` to continue reading. The `session_end` leg deletes the temp directory. Internal toolsets (`memory`, `plan`, `tasks`, `think`, …) are not affected. | +| `elide_repeated_tool_results` | `tool_response_transform`, `session_end`, `after_compaction`, `session_start` | _none_ | Opt-in. When a read-only tool from the `filesystem`, `lsp`, `rag`, `memory` or `git` categories returns output byte-for-byte identical to what the model already saw for the same arguments in this session, the payload is replaced with a one-line marker. **Not a cache**: the tool always runs and its fresh output is what gets hashed, so a changed file can never be served stale — the saving is in tokens, not latency. Only acts on payloads smaller than the `limit_large_tool_results` threshold, since that always-on hook is injected first and its rewrite wins. The `session_end` / `after_compaction` / `session_start` legs drop per-session state; compaction in particular removes the payload a fingerprint stands for. See [`examples/elide_repeated_tool_results.yaml`](https://github.com/docker/docker-agent/blob/main/examples/elide_repeated_tool_results.yaml). | | `safer_shell` | `pre_tool_use` | _none_ | **Deprecated compatibility shim.** The runtime now classifies every shell command natively (`safe` / `destructive` / `unknown`) and gates it through the session's [safety mode](../permissions/index.md#safety-modes), so this builtin no longer emits verdicts. Pinned entries keep working as pure labellers that attach classification metadata (`safety_label`, `blast_radius`, `category`, `reason`) to the call. Filters by tool name internally (no-op for non-shell calls). | | `unload` | `on_agent_switch` | _none_ | POSTs `{"model": ""}` to each of the previous agent's DMR model endpoints (`/_unload` by default, overridable per-model via `unload_api`) to free the GPU/RAM the just-departing model was holding. Pure HTTP — reads the model snapshot the runtime ships on `on_agent_switch` and depends on no provider-specific runtime state. Non-DMR providers (OpenAI, Anthropic, …) are silently skipped, so cross-provider chains are safe. Errors are logged and swallowed; agent switching never blocks on a slow or unreachable engine (each call has a 10 s timeout). See [`examples/unload_on_switch.yaml`](https://github.com/docker/docker-agent/blob/main/examples/unload_on_switch.yaml). | @@ -310,10 +311,10 @@ In addition to the common fields, each event ships its own payload: | Event | Extra fields | | --------------------------- | --------------------------------------------------------------------------------------------------------------------- | -| `pre_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input` | -| `tool_response_transform` | `tool_name`, `tool_use_id`, `tool_input`, `tool_response` | -| `post_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input`, `tool_response`, `tool_error` | -| `permission_request` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input` | +| `pre_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input`, `tool_category`, `tool_read_only` | +| `tool_response_transform` | `tool_name`, `tool_use_id`, `tool_input`, `tool_response`, `tool_category`, `tool_read_only` | +| `post_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input`, `tool_response`, `tool_error`, `tool_category`, `tool_read_only` | +| `permission_request` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input`, `tool_category`, `tool_read_only` | | `session_start` | `source` — one of `startup`, `resume`, `clear`, `compact` | | `user_prompt_submit` | `prompt` — the text the user just submitted | | `user_steering_messages_submit` | `steering_messages` — the drained steering messages, in submission order | diff --git a/examples/elide_repeated_tool_results.yaml b/examples/elide_repeated_tool_results.yaml new file mode 100644 index 0000000000..629522b8fb --- /dev/null +++ b/examples/elide_repeated_tool_results.yaml @@ -0,0 +1,50 @@ +#!/usr/bin/env docker-agent run +# +# Opt in to eliding repeated read-only tool results. +# +# When a read-only tool returns output byte-for-byte identical to what the model +# already saw for the same arguments in this session, the payload is replaced +# with a one-line marker instead of being repeated. An agent that re-reads the +# same file across turns pays for it once. +# +# This is NOT a cache: the tool always runs and its fresh output is what gets +# hashed, so a changed file can never be served stale. The saving is in tokens, +# not latency. +# +# Whether the marker is good for model behaviour is unmeasured — some models may +# re-request the file anyway — which is why this is opt-in rather than on by +# default. + +agents: + root: + model: claude + description: An agent that does not pay twice for the same file. + instruction: | + You are a helpful assistant working in a code repository. + Read files as you need them. + toolsets: + - type: filesystem + hooks: + tool_response_transform: + - matcher: "*" + hooks: + - type: builtin + command: elide_repeated_tool_results + # State is per session. Wire all three cleanup legs: session_end frees it, + # and compaction or a context reset removes the very messages a + # fingerprint stands for — keeping it would tell the model "nothing has + # changed" about bytes it can no longer see. + session_end: + - type: builtin + command: elide_repeated_tool_results + after_compaction: + - type: builtin + command: elide_repeated_tool_results + session_start: + - type: builtin + command: elide_repeated_tool_results + +models: + claude: + provider: anthropic + model: claude-sonnet-4-5 diff --git a/pkg/hooks/builtins/builtins.go b/pkg/hooks/builtins/builtins.go index 3a3c512bcc..7a4f6e1921 100644 --- a/pkg/hooks/builtins/builtins.go +++ b/pkg/hooks/builtins/builtins.go @@ -30,6 +30,11 @@ // - limit_large_tool_results // (tool_response_transform) — store oversized tool output in a temp file // and replace it with a bounded tail plus notice +// - elide_repeated_tool_results +// (tool_response_transform, session_end) — replace a read-only tool's +// output with a short marker when it is byte-for-byte identical to what +// the model already saw for the same arguments in this session. The tool +// always runs, so this cannot serve stale data; it saves tokens, not I/O. // - safer_shell (pre_tool_use) — deprecated labeller // shim. The runtime classifies shell commands // natively via pkg/safety; pinned entries only @@ -112,6 +117,7 @@ func Register(r *hooks.Registry, opts ...Option) error { r.RegisterBuiltin(MaxIterations, maxIterations), r.RegisterBuiltin(RedactSecrets, redactSecrets), r.RegisterBuiltin(LimitLargeToolResults, limitLargeToolResults), + r.RegisterBuiltin(ElideRepeatedToolResults, elideRepeatedToolResults), r.RegisterBuiltin(SaferShell, saferShell), r.RegisterBuiltin(HTTPPost, newHTTPPost(o.httpPostClient)), r.RegisterBuiltin(Unload, unload), diff --git a/pkg/hooks/builtins/elide_repeated_tool_results.go b/pkg/hooks/builtins/elide_repeated_tool_results.go new file mode 100644 index 0000000000..578e8f30ed --- /dev/null +++ b/pkg/hooks/builtins/elide_repeated_tool_results.go @@ -0,0 +1,266 @@ +package builtins + +import ( + "context" + "crypto/sha256" + "encoding/json" + "fmt" + "log/slog" + "slices" + "sync" + + "github.com/docker/docker-agent/pkg/hooks" +) + +// ElideRepeatedToolResults is the registered name of the builtin +// tool_response_transform hook that stops re-sending a read-only tool's output +// when it is byte-for-byte identical to what the model already saw earlier in +// the same session. +// +// # Why this cannot serve stale data +// +// This is deliberately NOT a cache. The tool always executes and its fresh +// output is always what gets hashed; the hook only decides whether to repeat +// bytes the model has already been shown. There is no stored payload to go +// stale, no expiry to tune, and no invalidation to get wrong: if the file (or +// whatever the tool reads) changed by even one byte, the hashes differ and the +// full new output is passed through untouched. +// +// The saving is in tokens, not in I/O — a repeated read_file result becomes a +// one-line marker. Latency is unchanged because the tool still runs. +// +// # Effective range +// +// [LimitLargeToolResults] is auto-injected at the FRONT of +// tool_response_transform and the executor applies the first non-nil rewrite in +// config order, so whenever that builtin fires (results over ~50 KiB or 2000 +// lines) its truncation wins and an elision marker would be discarded. Nothing +// a user writes in YAML can precede an auto-injected entry, so this builtin +// deliberately declines to act on payloads that large: eliding them is not +// possible today, and recording a fingerprint for them would only waste memory. +// +// The effective window is therefore (len(marker), maxToolCallResultBytes). +// Repeats above it stay bounded by limit_large_tool_results, which caps a single +// result but not the cost of repeating it. +const ElideRepeatedToolResults = "elide_repeated_tool_results" + +// elidableCategories lists the tool categories this builtin will act on. +// +// ReadOnlyHint alone is not a purity signal in this codebase: several built-in +// tools set it for approval-gating reasons while still having effects +// (create_todo, update_todos, handoff), and for MCP tools it is copied verbatim +// from the remote server — so a third-party server could self-declare it and +// have its repeated output suppressed from the transcript and the persisted +// session. Pairing the hint with a category the agent's own toolsets own keeps +// that decision local, the same way limit_large_tool_results scopes itself. +var elidableCategories = map[string]bool{ + "filesystem": true, + "lsp": true, + "rag": true, + "memory": true, + "git": true, +} + +const ( + // maxElideKeysPerSession bounds per-session memory. A session that calls + // read-only tools with thousands of distinct argument sets stops recording + // new fingerprints rather than growing without limit; already-recorded keys + // keep working. Each entry is a 32-byte hash plus a map key. + maxElideKeysPerSession = 4096 + + // maxElideSessions bounds how many sessions are tracked at once. Cleanup is + // driven by a session_end entry the operator has to wire up, and a session + // that ends abnormally never fires it, so a long-lived `serve api` process + // would otherwise accumulate one map per session it ever saw. Past the cap + // the oldest tracked session is dropped; it simply stops eliding. + maxElideSessions = 256 +) + +// elideState remembers, per session, the fingerprint of the most recent output +// seen for each (tool, arguments) pair. +// +// Package-level state because builtins are registered as plain functions and +// have nowhere else to live. Entries are dropped on session_end, on compaction, +// and on session_start; the session count is capped for the cases where none of +// those fire. +type elideState struct { + mu sync.Mutex + // seen maps session ID -> call key -> sha256 of the last output. + seen map[string]map[string][sha256.Size]byte + // order records session IDs in first-seen order so the oldest can be + // dropped when the cap is reached. + order []string +} + +var elideStore = &elideState{seen: make(map[string]map[string][sha256.Size]byte)} + +// elideRepeatedToolResults is the [hooks.BuiltinFunc] registered under +// [ElideRepeatedToolResults]. It dispatches on the event so one YAML entry can +// cover both the transform leg and the session_end cleanup. +func elideRepeatedToolResults(_ context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) { + if in == nil { + return nil, nil + } + switch in.HookEventName { + case hooks.EventToolResponseTransform: + return elideRepeatedToolResponse(in), nil + case hooks.EventSessionEnd: + elideStore.forget(in.SessionID) + return nil, nil + case hooks.EventAfterCompaction: + // Compaction drops the messages before the kept-tail boundary, so a + // fingerprint can outlive the payload it stands for. Keeping it would + // tell the model "nothing has changed" about bytes it can no longer + // see, permanently — only a byte change would ever release the payload + // again. Forgetting is the conservative direction: at worst one full + // result is re-sent. + elideStore.forget(in.SessionID) + return nil, nil + case hooks.EventSessionStart: + // "compact" and "clear" rebuild the context the same way; "startup" and + // "resume" begin one, where stale state from a previous process would + // be equally wrong. + elideStore.forget(in.SessionID) + return nil, nil + default: + // Lenient on misconfiguration, matching redact_secrets: log the typo + // but never fail the run loop over a misplaced hook entry. + slog.Warn("elide_repeated_tool_results builtin invoked under unsupported event; no-op", + "event", in.HookEventName) + return nil, nil + } +} + +// elideRepeatedToolResponse returns a marker in place of payloads that repeat +// an earlier identical result, or nil to leave the response untouched. +func elideRepeatedToolResponse(in *hooks.Input) *hooks.Output { + // Only tools the author declared read-only are eligible. A tool with side + // effects may legitimately return identical output for two calls that each + // did something (e.g. an append that was then undone), so eliding the + // second would hide a real event. + // + // The category check is the second half of that: the hint is a declaration + // rather than a proof, and for MCP tools the remote server supplies it. + if !in.ToolReadOnly || !elidableCategories[in.ToolCategory] { + return nil + } + // An error result is diagnostic: the model needs it every time, and a + // repeated identical failure is itself information. + if in.ToolError { + return nil + } + // Without a session there is nothing to scope the state to. + if in.SessionID == "" { + return nil + } + + payload, ok := in.ToolResponse.(string) + if !ok { + return nil + } + + // Above this, limit_large_tool_results truncates first and wins, so an + // elision marker would be built and then discarded. + if len(payload) >= maxToolCallResultBytes { + return nil + } + + marker := elideMarker(in.ToolName, len(payload)) + // Eliding must actually shrink the conversation. Comparing against the real + // marker keeps that true by construction, including for the long tool names + // (MCP calls run to 30+ characters) where a fixed threshold does not. + if len(payload) <= len(marker) { + return nil + } + + key, ok := elideCallKey(in.ToolName, in.ToolInput) + if !ok { + return nil + } + + if !elideStore.observe(in.SessionID, key, sha256.Sum256([]byte(payload))) { + return nil + } + + return &hooks.Output{ + HookSpecificOutput: &hooks.HookSpecificOutput{ + HookEventName: hooks.EventToolResponseTransform, + UpdatedToolResponse: &marker, + }, + } +} + +// elideMarker is what replaces an elided payload. Worded to say explicitly that +// the tool ran, so the model does not treat it as a cache hit of unknown age. +func elideMarker(toolName string, payloadBytes int) string { + return fmt.Sprintf( + "[docker-agent] The %s tool ran and returned output byte-for-byte identical to its "+ + "earlier result for these same arguments in this session, so the %d-byte payload is "+ + "not repeated here. Nothing has changed since you last saw it.", + toolName, payloadBytes) +} + +// elideCallKey fingerprints a call as its tool name plus its arguments. +// [encoding/json] sorts map keys, so the result does not depend on Go's +// randomized map iteration order. Arguments that cannot be marshalled yield +// ok=false, which makes the caller leave the response untouched. +func elideCallKey(tool string, args map[string]any) (string, bool) { + encoded, err := json.Marshal(args) + if err != nil { + return "", false + } + h := sha256.New() + h.Write([]byte(tool)) + h.Write([]byte{0}) + h.Write(encoded) + return string(h.Sum(nil)), true +} + +// observe records sum as the latest output fingerprint for (session, key) and +// reports whether it repeats what was already recorded. A mismatch overwrites +// the stored fingerprint, so the *next* identical call elides. +func (s *elideState) observe(sessionID, key string, sum [sha256.Size]byte) bool { + s.mu.Lock() + defer s.mu.Unlock() + + perSession, ok := s.seen[sessionID] + if !ok { + // Drop the oldest tracked session when the cap is reached: cleanup + // depends on a session_end entry the operator may not have wired, and a + // session that ends abnormally never fires one. + for len(s.order) >= maxElideSessions { + delete(s.seen, s.order[0]) + s.order = s.order[1:] + } + perSession = make(map[string][sha256.Size]byte, 1) + s.seen[sessionID] = perSession + s.order = append(s.order, sessionID) + } + + previous, seen := perSession[key] + if seen { + if previous == sum { + return true + } + perSession[key] = sum + return false + } + + // New key: respect the per-session cap. Declining to record simply means + // this call is never elided — correctness is unaffected. + if len(perSession) < maxElideKeysPerSession { + perSession[key] = sum + } + return false +} + +// forget drops all state for a session. +func (s *elideState) forget(sessionID string) { + s.mu.Lock() + defer s.mu.Unlock() + if _, ok := s.seen[sessionID]; !ok { + return + } + delete(s.seen, sessionID) + s.order = slices.DeleteFunc(s.order, func(id string) bool { return id == sessionID }) +} diff --git a/pkg/hooks/builtins/elide_repeated_tool_results_test.go b/pkg/hooks/builtins/elide_repeated_tool_results_test.go new file mode 100644 index 0000000000..70cc844849 --- /dev/null +++ b/pkg/hooks/builtins/elide_repeated_tool_results_test.go @@ -0,0 +1,395 @@ +package builtins + +import ( + "fmt" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks" +) + +// bigPayload returns a payload comfortably above the marker length and below +// the limit_large_tool_results threshold. +func bigPayload(seed string) string { + return seed + strings.Repeat("x", 1024) +} + +// forgetAllElideState resets the package-level store between tests. These tests +// deliberately do not run in parallel with each other: they share that store, +// which is the same state the runtime shares across a process. +func forgetAllElideState() { + elideStore.mu.Lock() + defer elideStore.mu.Unlock() + elideStore.seen = make(map[string]map[string][32]byte) + elideStore.order = nil +} + +// elideStoreSessions reports how many sessions are currently tracked. +func elideStoreSessions() int { + elideStore.mu.Lock() + defer elideStore.mu.Unlock() + return len(elideStore.seen) +} + +// elideStoreLen reports how many call keys are recorded for a session. +func elideStoreLen(sessionID string) int { + elideStore.mu.Lock() + defer elideStore.mu.Unlock() + return len(elideStore.seen[sessionID]) +} + +func transformInput(sessionID, tool, payload string, args map[string]any) *hooks.Input { + return &hooks.Input{ + HookEventName: hooks.EventToolResponseTransform, + SessionID: sessionID, + ToolName: tool, + ToolCategory: "filesystem", + ToolReadOnly: true, + ToolInput: args, + ToolResponse: payload, + } +} + +func elide(t *testing.T, in *hooks.Input) *hooks.Output { + t.Helper() + out, err := elideRepeatedToolResults(t.Context(), in, nil) + require.NoError(t, err) + return out +} + +func TestElideRepeatedToolResults_FirstCallPassesThrough(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + + out := elide(t, transformInput("s1", "read_file", payload, map[string]any{"path": "a.txt"})) + assert.Nil(t, out, "the first result must reach the model in full") +} + +func TestElideRepeatedToolResults_IdenticalRepeatIsElided(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + args := map[string]any{"path": "a.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, args))) + + out := elide(t, transformInput("s1", "read_file", payload, args)) + require.NotNil(t, out) + require.NotNil(t, out.HookSpecificOutput) + require.NotNil(t, out.HookSpecificOutput.UpdatedToolResponse) + + got := *out.HookSpecificOutput.UpdatedToolResponse + assert.NotEqual(t, payload, got) + assert.Less(t, len(got), len(payload), "the marker must be smaller than the payload it replaces") + assert.Contains(t, got, "read_file") + assert.Contains(t, got, "identical") +} + +// THE consistency property: the payload is only ever elided when the tool's +// fresh output is byte-for-byte identical to what the model already saw. The +// tool always executes, so a changed file can never be served from cache. +func TestElideRepeatedToolResults_ChangedOutputIsNeverElided(t *testing.T) { + forgetAllElideState() + args := map[string]any{"path": "a.txt"} + first := bigPayload("version-one") + second := bigPayload("version-two") + + require.Nil(t, elide(t, transformInput("s1", "read_file", first, args))) + + out := elide(t, transformInput("s1", "read_file", second, args)) + assert.Nil(t, out, "changed output must always reach the model in full") + + // And the new output becomes the baseline, so a repeat of *it* elides + // while a return to the old content does not. + require.NotNil(t, elide(t, transformInput("s1", "read_file", second, args))) + assert.Nil(t, elide(t, transformInput("s1", "read_file", first, args)), + "reverting to earlier content must reach the model in full") +} + +func TestElideRepeatedToolResults_NonReadOnlyToolIsNeverElided(t *testing.T) { + forgetAllElideState() + payload := bigPayload("side effects") + args := map[string]any{"cmd": "date"} + + in := transformInput("s1", "shell", payload, args) + in.ToolReadOnly = false + require.Nil(t, elide(t, in)) + + in2 := transformInput("s1", "shell", payload, args) + in2.ToolReadOnly = false + assert.Nil(t, elide(t, in2), "a tool with side effects must never be elided") +} + +func TestElideRepeatedToolResults_ErrorResultIsNeverElided(t *testing.T) { + forgetAllElideState() + payload := bigPayload("boom") + args := map[string]any{"path": "a.txt"} + + in := transformInput("s1", "read_file", payload, args) + in.ToolError = true + require.Nil(t, elide(t, in)) + + in2 := transformInput("s1", "read_file", payload, args) + in2.ToolError = true + assert.Nil(t, elide(t, in2)) +} + +func TestElideRepeatedToolResults_DifferentArgsAreDistinct(t *testing.T) { + forgetAllElideState() + payload := bigPayload("same bytes") + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, map[string]any{"path": "a.txt"}))) + assert.Nil(t, elide(t, transformInput("s1", "read_file", payload, map[string]any{"path": "b.txt"})), + "a different argument set is a different call") +} + +// Key building must not depend on Go's randomized map iteration order. +func TestElideRepeatedToolResults_ArgOrderIsIrrelevant(t *testing.T) { + forgetAllElideState() + payload := bigPayload("stable") + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, + map[string]any{"path": "a.txt", "line": 1, "limit": 20}))) + + for range 20 { + out := elide(t, transformInput("s1", "read_file", payload, + map[string]any{"limit": 20, "line": 1, "path": "a.txt"})) + require.NotNil(t, out, "identical args in any map order must be the same key") + } +} + +func TestElideRepeatedToolResults_SessionsAreIsolated(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + args := map[string]any{"path": "a.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, args))) + assert.Nil(t, elide(t, transformInput("s2", "read_file", payload, args)), + "another session has not seen this output") +} + +func TestElideRepeatedToolResults_SmallPayloadNotWorthEliding(t *testing.T) { + forgetAllElideState() + small := "tiny" + args := map[string]any{"path": "a.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", small, args))) + assert.Nil(t, elide(t, transformInput("s1", "read_file", small, args)), + "eliding a payload smaller than the marker would cost tokens, not save them") +} + +func TestElideRepeatedToolResults_SessionEndForgetsState(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + args := map[string]any{"path": "a.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, args))) + require.NotNil(t, elide(t, transformInput("s1", "read_file", payload, args))) + + _, err := elideRepeatedToolResults(t.Context(), &hooks.Input{ + HookEventName: hooks.EventSessionEnd, + SessionID: "s1", + }, nil) + require.NoError(t, err) + + assert.Nil(t, elide(t, transformInput("s1", "read_file", payload, args)), + "state must be dropped when the session ends") +} + +func TestElideRepeatedToolResults_PerSessionKeyCapIsBounded(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + + // Fill past the cap with distinct argument sets. + for i := range maxElideKeysPerSession + 50 { + elide(t, transformInput("s1", "read_file", payload, map[string]any{"path": i})) + } + assert.LessOrEqual(t, elideStoreLen("s1"), maxElideKeysPerSession, + "per-session key count must stay bounded") +} + +// Parallel tool calls dispatch this hook concurrently; run under -race. +func TestElideRepeatedToolResults_ConcurrentDispatch(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + + var wg sync.WaitGroup + for i := range 32 { + wg.Go(func() { + for range 8 { + _, err := elideRepeatedToolResults(t.Context(), + transformInput("s1", "read_file", payload, map[string]any{"path": i % 4}), nil) + assert.NoError(t, err) + } + }) + } + wg.Wait() +} + +func TestElideRepeatedToolResults_IsRegistered(t *testing.T) { + forgetAllElideState() + reg := hooks.NewRegistry() + require.NoError(t, Register(reg)) + + handler, ok := reg.LookupBuiltin(ElideRepeatedToolResults) + require.Truef(t, ok, "builtin %q must be registered", ElideRepeatedToolResults) + + payload := bigPayload("contents") + args := map[string]any{"path": "a.txt"} + + first, err := handler(t.Context(), transformInput("s9", "read_file", payload, args), nil) + require.NoError(t, err) + require.Nil(t, first) + + second, err := handler(t.Context(), transformInput("s9", "read_file", payload, args), nil) + require.NoError(t, err) + require.NotNil(t, second) +} + +func TestElideRepeatedToolResults_UnsupportedEventIsNoOp(t *testing.T) { + forgetAllElideState() + out, err := elideRepeatedToolResults(t.Context(), &hooks.Input{ + HookEventName: hooks.EventTurnStart, + SessionID: "s1", + }, nil) + require.NoError(t, err) + assert.Nil(t, out) +} + +func TestElideRepeatedToolResults_NilInput(t *testing.T) { + forgetAllElideState() + out, err := elideRepeatedToolResults(t.Context(), nil, nil) + require.NoError(t, err) + assert.Nil(t, out) +} + +// limit_large_tool_results is auto-injected at the FRONT of +// tool_response_transform and the first non-nil rewrite in config order wins, so +// for payloads it handles an elision marker would be built and then thrown away. +// Declining to act keeps the state honest instead of recording a fingerprint for +// a marker that never reaches the model. +func TestElideRepeatedToolResults_DeclinesPayloadsLimitLargeWillTruncate(t *testing.T) { + forgetAllElideState() + + huge := strings.Repeat("x", maxToolCallResultBytes+1) + args := map[string]any{"path": "big.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", huge, args))) + assert.Nil(t, elide(t, transformInput("s1", "read_file", huge, args)), + "a payload limit_large_tool_results will truncate must not be elided") + assert.Zero(t, elideStoreLen("s1"), + "and must not consume state for a marker that would be discarded") +} + +// Compaction drops the messages a fingerprint stands for. Keeping it would tell +// the model "nothing has changed" about bytes it can no longer see — for the +// rest of the session, since only a byte change would release the payload again. +func TestElideRepeatedToolResults_CompactionForgetsState(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + args := map[string]any{"path": "a.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, args))) + require.NotNil(t, elide(t, transformInput("s1", "read_file", payload, args))) + + _, err := elideRepeatedToolResults(t.Context(), &hooks.Input{ + HookEventName: hooks.EventAfterCompaction, + SessionID: "s1", + }, nil) + require.NoError(t, err) + + assert.Nil(t, elide(t, transformInput("s1", "read_file", payload, args)), + "after compaction the payload must be re-sent, not marked unchanged") +} + +func TestElideRepeatedToolResults_SessionStartForgetsState(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + args := map[string]any{"path": "a.txt"} + + require.Nil(t, elide(t, transformInput("s1", "read_file", payload, args))) + require.NotNil(t, elide(t, transformInput("s1", "read_file", payload, args))) + + for _, source := range []string{"compact", "clear", "resume"} { + _, err := elideRepeatedToolResults(t.Context(), &hooks.Input{ + HookEventName: hooks.EventSessionStart, + SessionID: "s1", + Source: source, + }, nil) + require.NoError(t, err) + + assert.Nilf(t, elide(t, transformInput("s1", "read_file", payload, args)), + "session_start %q rebuilds the context, so state must be dropped", source) + require.NotNil(t, elide(t, transformInput("s1", "read_file", payload, args))) + } +} + +// ReadOnlyHint is a declaration, not a proof: built-in tools set it for +// approval-gating reasons while still having effects, and for MCP tools the +// remote server supplies it. +func TestElideRepeatedToolResults_CategoryMustAlsoBeElidable(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + args := map[string]any{"q": "x"} + + // A read-only-declaring tool from a category this builtin does not own. + mk := func() *hooks.Input { + in := transformInput("s1", "search", payload, args) + in.ToolCategory = "mcp" + return in + } + require.Nil(t, elide(t, mk())) + assert.Nil(t, elide(t, mk()), + "a self-declared read-only MCP tool must not be able to suppress its own output") + + // An empty category (tool unknown to the agent) is equally inert. + forgetAllElideState() + mkEmpty := func() *hooks.Input { + in := transformInput("s1", "read_file", payload, args) + in.ToolCategory = "" + return in + } + require.Nil(t, elide(t, mkEmpty())) + assert.Nil(t, elide(t, mkEmpty())) +} + +// A fixed byte threshold does not hold for long tool names: the marker embeds +// the name, so an MCP call at 30+ characters produces a marker longer than the +// payload it would replace. +func TestElideRepeatedToolResults_NeverGrowsTheConversation(t *testing.T) { + forgetAllElideState() + + const longName = "mcp__github__list_pull_requests" + // Above the old fixed 256-byte threshold would have been elided; the marker + // for a name this long is larger still. + payload := strings.Repeat("y", 250) + args := map[string]any{"path": "a"} + + require.Greater(t, len(elideMarker(longName, len(payload))), len(payload), + "fixture must exercise the case where the marker is the larger of the two") + + in := func() *hooks.Input { + i := transformInput("s1", longName, payload, args) + i.ToolCategory = "filesystem" + return i + } + require.Nil(t, elide(t, in())) + assert.Nil(t, elide(t, in()), + "eliding must never replace a payload with something longer") +} + +// Cleanup depends on a session_end entry the operator has to wire up, and an +// abnormally-ended session never fires one. +func TestElideRepeatedToolResults_SessionCountIsBounded(t *testing.T) { + forgetAllElideState() + payload := bigPayload("contents") + + for i := range maxElideSessions + 100 { + elide(t, transformInput(fmt.Sprintf("s%d", i), "read_file", payload, map[string]any{"path": "a"})) + } + assert.LessOrEqual(t, elideStoreSessions(), maxElideSessions, + "tracked session count must stay bounded without session_end") +} diff --git a/pkg/hooks/types.go b/pkg/hooks/types.go index 1a7c2990dd..d10e0c3a45 100644 --- a/pkg/hooks/types.go +++ b/pkg/hooks/types.go @@ -274,6 +274,19 @@ type Input struct { ToolUseID string `json:"tool_use_id,omitempty"` ToolInput map[string]any `json:"tool_input,omitempty"` + // ToolReadOnly mirrors the dispatching tool's ReadOnlyHint annotation on + // every tool event, so a hook can tell a tool that only observes from one + // that mutates something. False whenever the hint is absent or the tool is + // unknown to the agent, which keeps consumers fail-safe: a hook that acts + // only on read-only tools does nothing when the declaration is missing. + // + // Treat it as a declaration, not a proof. Several built-in tools set the + // hint for approval-gating reasons while still having effects (create_todo, + // handoff), and for MCP tools it is copied verbatim from the remote server — + // so a third-party server decides its own value. A hook that must not act on + // a tool with side effects should pair this with [Input.ToolCategory]. + ToolReadOnly bool `json:"tool_read_only,omitempty"` + // SafetyPolicy mirrors the session's effective safety mode // (strict / balanced / autonomous, empty for the legacy default; // see [github.com/docker/docker-agent/pkg/session.SafetyPolicy]) diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index 4a2f154b98..032db01183 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -618,7 +618,7 @@ func (c *call) consultPreToolUsePreYolo(ctx context.Context) *hooks.Result { if c.d.Hooks == nil { return nil } - c.preYoloResult = c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPreToolUsePreYolo, NewHooksInput(c.sess, c.tc)) + c.preYoloResult = c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPreToolUsePreYolo, c.hooksInput()) return c.preYoloResult } @@ -640,7 +640,7 @@ func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOut return CallOutcome{}, false } - result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPreToolUse, NewHooksInput(c.sess, c.tc)) + result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPreToolUse, c.hooksInput()) if result == nil { return CallOutcome{}, false } @@ -1077,8 +1077,7 @@ func (c *call) applyToolResponseTransform(ctx context.Context, payload string, i if c.d.Hooks == nil { return payload } - in := NewPostToolHooksInput(c.sess, c.tc, &tools.ToolCallResult{Output: payload, IsError: isError}) - in.ToolCategory = c.tool.Category + in := c.postToolHooksInput(&tools.ToolCallResult{Output: payload, IsError: isError}) result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventToolResponseTransform, in) if result == nil || result.UpdatedToolResponse == nil { return payload @@ -1086,6 +1085,30 @@ func (c *call) applyToolResponseTransform(ctx context.Context, payload string, i return *result.UpdatedToolResponse } +// hooksInput builds a tool-event [hooks.Input] carrying this call's tool +// metadata. Every tool event goes through here so the metadata cannot be +// present on one lane and missing on another. +// +// Category and ReadOnlyHint are zero when !c.available (the tool isn't in the +// agent's toolset), which is the fail-safe direction: consumers keyed on +// read-only-ness stay inert rather than guessing. +func (c *call) hooksInput() *hooks.Input { + in := NewHooksInput(c.sess, c.tc) + in.ToolCategory = c.tool.Category + in.ToolReadOnly = c.tool.Annotations.ReadOnlyHint + return in +} + +// postToolHooksInput is [call.hooksInput] plus the tool result. +func (c *call) postToolHooksInput(res *tools.ToolCallResult) *hooks.Input { + in := c.hooksInput() + if res != nil { + in.ToolResponse = res.Output + in.ToolError = res.IsError + } + return in +} + // translateError converts a tool-handler error into a [tools.ToolCallResult] // suitable for the conversation, while annotating the span. Context-cancel // errors are reported as user cancellation (Ok status); everything else is @@ -1142,7 +1165,7 @@ func (c *call) postHook(ctx context.Context, res *tools.ToolCallResult) (stop bo if c.d.Hooks == nil { return false, "" } - result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPostToolUse, NewPostToolHooksInput(c.sess, c.tc, res)) + result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPostToolUse, c.postToolHooksInput(res)) if result == nil || result.Allowed { return false, "" }