feat(hooks): elide repeated read-only tool results instead of re-sending them

[dwin-gharibi]

Adds the elide_repeated_tool_results builtin. When a read-only tool returns output byte-for-byte
identical to what the model was already shown for the same arguments in the same session, the
payload is replaced with a one-line marker instead of being repeated.

hooks:
  tool_response_transform:
    - { type: builtin, command: elide_repeated_tool_results }
  session_end:
    - { type: builtin, command: elide_repeated_tool_results }

Closes #3939.

This is deliberately not a cache

The obvious version of this feature caches results and skips re-execution — and then hands the
agent a stale file when something changes underneath it. That is the failure mode worth designing
away, so this does not store payloads at all:

1. the tool always executes;
2. its fresh output is hashed;
3. the payload is elided only if that hash matches what the model already saw.

There is no stored payload to go stale, no expiry to tune, and no invalidation to get wrong. A
one-byte change means the hashes differ and the full new output passes through untouched. The test
ChangedOutputIsNeverElided pins this, including the harder direction: after content changes, the
new output becomes the baseline, and reverting to the older content is also passed through in
full rather than matching a remembered older hash.

The honest trade-off: the saving is tokens, not latency. The tool still runs. A 40 KiB
read_file repeated five times across a session costs 40 KiB once instead of five times, and the
context window it would have consumed is what pulls compaction forward.

What the model sees

[docker-agent] The read_file tool ran and returned output byte-for-byte identical to its earlier
result for these same arguments in this session, so the 41,232-byte payload is not repeated here.
Nothing has changed since you last saw it.

Worded to say explicitly that the tool ran, so the model does not treat it as a cache hit of
unknown age.

Scope decisions

Rule	Why
Read-only tools only	A tool with side effects can return identical output for two calls that each did something; eliding the second would hide a real event
Never elide errors	A repeated identical failure is itself information
Never elide below 256 bytes	The marker would cost more tokens than the payload it replaces
Per-session state, dropped on session_end	Two agents in one process must not cross-contaminate
Opt-in	It changes what the model sees; that is not a default to impose

Read-only-ness comes from the tool's own ReadOnlyHint annotation rather than a hard-coded name
list, so it works for MCP tools that declare it too (tools.ToolAnnotations is
mcp.ToolAnnotations).

Implementation

Three small pieces, all at existing seams:

pkg/hooks/types.go — new Input.ToolReadOnly field mirroring the tool's ReadOnlyHint. It
is false whenever the hint is absent or the tool is unknown to the agent, which is the fail-safe
direction: a consumer keyed on read-only-ness stays inert rather than guessing.

pkg/runtime/toolexec/dispatcher.go — one line populating it in
applyToolResponseTransform, where c.tool is already in hand.

pkg/hooks/builtins/elide_repeated_tool_results.go — the builtin. Dispatches on event so a
single name covers both legs, the same pattern redact_secrets uses. State is a package-level
per-session map of (tool, args) → sha256(last output), mutex-guarded because parallel tool calls
dispatch hooks concurrently. Keys are sha256(toolName ‖ 0x00 ‖ json.Marshal(args)); encoding/json
sorts map keys, so the key does not depend on Go's randomized map iteration order — there's a test
that hammers that 20×.

Per-session keys are capped at 4096. Past the cap, new fingerprints are simply not recorded, so
those calls are never elided — bounded memory with no correctness impact.

Tests

15 tests in pkg/hooks/builtins/elide_repeated_tool_results_test.go:

• ChangedOutputIsNeverElided — the consistency property, both directions
• FirstCallPassesThrough, IdenticalRepeatIsElided — the happy path
• NonReadOnlyToolIsNeverElided, ErrorResultIsNeverElided — the scope rules
• DifferentArgsAreDistinct, ArgOrderIsIrrelevant — key correctness
• SessionsAreIsolated, SessionEndForgetsState, PerSessionKeyCapIsBounded — state lifecycle
• ConcurrentDispatch — runs under -race
• IsRegistered, UnsupportedEventIsNoOp, NilInput, SmallPayloadNotWorthEliding

These tests deliberately do not use t.Parallel(): they share the package-level store, which is
the same state the runtime shares across a process. There's a comment saying so, so nobody
"fixes" it later.

Verification

Toolchain go1.26.5, darwin/arm64.

Check	Result
go test ./pkg/hooks/...	ok
go test ./pkg/runtime/...	ok (all subpackages — the dispatcher change)
go test -race -count=1 ./pkg/hooks/...	ok
golangci-lint run ./pkg/hooks/... ./pkg/runtime/toolexec/... (v2.12.2, CI's pin)	0 issues
go run ./lint .	1768 files, no offenses
gofmt -l, go build ./...	clean
go test ./...	only pkg/teamloader fails — pre-existing (Google Cloud ADC), unrelated

Not in this PR

• No eval evidence that the marker is good for model behaviour. It might make some models
  re-read anyway. That is the main open question and the reason this ships opt-in rather than
  auto-injected; it wants an eval pass before anyone considers a default.
• No agent-level flag. A hook entry is enough to try it; promoting it to a flag is a separate
  decision once behaviour is measured.
• Latency is untouched by design. If skipping execution is ever wanted, that is a different
  feature with a real invalidation problem to solve, and should be argued separately.

[docker-agent]

Assessment: 🟢 APPROVE

The implementation is well-structured and the design decisions are sound. The mutex-guarded package-level state, session isolation, cap enforcement (4096 keys), fail-safe defaults for unknown/unavailable tools, and the hash-based elision logic are all correct. The concurrency model handles concurrent tool_response_transform dispatches and concurrent session_end cleanup safely. The json.Marshal key stability claim holds because encoding/json sorts map keys recursively. No bugs introduced by this PR were found.

[aheritier]

Thanks for this — the central design decision is the right one, and I want to say so before the findings. Hashing fresh output and comparing, rather than storing and serving, sidesteps the invalidation problem entirely instead of trying to solve it, and ChangedOutputIsNeverElided pins that property in both directions including the revert case. The comments explain why throughout. That is a higher standard than most contributions arrive at.

I found two blocking issues, both against runtime behaviour that is on by default, and both reproduced.

1. [blocking] The marker is discarded for exactly the large results this targets.

ApplyAgentDefaults prepends limit_large_tool_results to ToolResponseTransform (pkg/hooks/builtins/builtins.go:167-170), and pkg/hooks/executor.go:535 applies only the first non-nil UpdatedToolResponse in config order. So whenever limit_large_tool_results fires (>50 KiB or >2000 lines) it wins and the elide marker is dropped. Driving the real executor with the effective config (user entry + prepended default) and a 60 KiB payload:

call 1 rewrite is limit_large notice: true
call 2 contains elide marker: false
call 2 contains limit_large notice: true   (51781 bytes)
elide store DID record a fingerprint anyway: 1 key

An 8 KiB repeat is elided, so the effective window is [256 B, 50 KiB and ≤2000 lines). #3939's motivating table says limit_large "bounds a single result; a 40 KiB result repeated five times still costs 5 × 40 KiB" — above the threshold that stays true after this PR. Worth a test, since the competing hook is injected unconditionally and cannot be removed from config.

2. [blocking] Compaction invalidates the "the model already saw it" premise, and nothing clears the store.

Compaction is on by default (pkg/runtime/runtime.go:687, pkg/agent/agent.go:363) and drops every message before FirstKeptEntry, replacing the prefix with a summary (pkg/runtime/session_compaction.go:183). elideStore is keyed on SessionID and cleared only on session_end (elide_repeated_tool_results.go:70-72), so after compaction the fingerprint survives while the payload is gone from context:

keys still recorded for s1 after compaction: 1
attempt 1 -> "...Nothing has changed since you last saw it."
attempt 2 -> (same)
attempt 3 -> (same)

The model is told nothing changed about bytes it can no longer see, permanently — only a byte change would ever release the payload. The same happens with max_old_tool_call_tokens, where truncateOldToolContent (pkg/session/session.go:2124) swaps older tool results for [content truncated]. Handling EventAfterCompaction (pkg/hooks/types.go:176) plus session_start with source=compact/clear would cover it.

3. [should-fix] ReadOnlyHint isn't a purity signal in this codebase, and for MCP it's untrusted input.

The scope rule assumes ToolReadOnly implies no side effects. It doesn't: pkg/tools/builtin/todo/todo.go:345,357,369 annotate create_todo/create_todos/update_todos with ReadOnlyHint: true // Technically not read-only but has practically no destructive side effects., and handoff.go:38 does the same. pkg/safety/safety.go:33 shows its real role is approval gating. For MCP tools the annotation is copied verbatim from the remote server (pkg/tools/mcp/mcp.go:682-683). Since the rewrite reaches the UI feed and the persisted session file (dispatcher.go:1071-1077), a third-party MCP server that self-declares readOnlyHint can suppress its own repeated output from the transcript. Opt-in mitigates, but a category allow-list alongside the hint — as limit_large_tool_results.go:38-43 does — would fence it properly, plus a note on the trust assumption.

4. [should-fix] Docs. A new builtin needs four updates that are currently complete for every other builtin: the table at docs/configuration/hooks/index.md:219; the builtin enumeration in the type description at agent-schema.json:1395; the tool_response_transform extra-fields row at docs/configuration/hooks/index.md:314 for tool_read_only (it's part of the JSON contract external command hooks receive); and an examples/*.yaml, which AGENTS.md requires and which redact_secrets_hooks.yaml / unload_on_switch.yaml / snapshot_hooks.yaml all have.

5. [should-fix] Input.ToolReadOnly doc is broader than the field. pkg/hooks/types.go:277-282 presents it as generally mirroring ReadOnlyHint, but the only assignment is dispatcher.go:1084 in applyToolResponseTransform. Hook authors on pre_tool_use/post_tool_use/permission_request get false, which the doc tells them means "the hint is absent". Either populate it in NewHooksInput — c.tool is in hand at every tool event — or scope the comment.

6. [should-fix] Session count is unbounded and cleanup is optional. maxElideKeysPerSession caps keys within a session, but nothing caps sessions, and freeing depends on the operator also wiring the session_end leg — optional here, unlike limit_large_tool_results, which is auto-injected on both legs. 50,000 distinct sessions retain 50,000 maps in my repro. Under serve api/serve mcp that's ~4096 entries per session held for process lifetime, plus every session that ends abnormally. An LRU cap on sessions, or hanging the store off the executor instead of the package, would close it.

7. [optional] minElidableBytes = 256 is below the marker's own size for long tool names. The marker is 234 B for read_file, 245 B for search_files_content, 255 B for a 30-char MCP name — so the saving at the boundary is 1–22 bytes and goes negative at ≥31 chars (mcp__github__list_pull_requests). Comparing len(marker) with len(payload) before returning would make the invariant hold by construction.

8. [optional] elide_repeated_tool_results.go:48-52 cites limit_large_tool_results as precedent for package-level state, but that builtin keeps none — its per-session scratch is a temp directory on disk.

9. [optional] Squash merge is off here, so all three commit subjects land in history. They're 115–120 chars, scoped by file path rather than component, and call the change "readonly tool caching" / "cache consistency" — which reads against the PR's own "deliberately not a cache" framing.

On whether to land this at all. The mechanism is sound; my hesitation is about sequencing. #3939 is self-filed with no maintainer comment, so there's no signal yet that this is wanted, and the PR itself names the decisive unknown: "No eval evidence that the marker is good for model behaviour. It might make some models re-read anyway." That's the whole bet — a model told "nothing has changed" may re-call with a slightly different argument, which changes the key, defeats the elision, and costs an extra round trip plus the full payload. With finding 1 narrowing the benefit to [256 B, 50 KiB) and finding 2 making the in-band failure silent context loss, I'd want the eval pass before the builtin lands.

A concrete suggestion: split the ToolReadOnly plumbing (pkg/hooks/types.go + pkg/runtime/toolexec/dispatcher.go, ~10 lines) into its own PR with finding 5 fixed. It's useful to hooks generally and should go in easily. Then bring the builtin back with 1 and 2 addressed and some eval numbers behind it.