feat(hooks): elide repeated read-only tool results instead of re-sending them - #3940
Open
dwin-gharibi wants to merge 3 commits into
Open
feat(hooks): elide repeated read-only tool results instead of re-sending them#3940dwin-gharibi wants to merge 3 commits into
dwin-gharibi wants to merge 3 commits into
Conversation
…hing for improving costs and tool calls count
…e elide hook to make sure cache consistency
…some edge case tests for new elide hook added
docker-agent
reviewed
Aug 7, 2026
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the
elide_repeated_tool_resultsbuiltin. When a read-only tool returns output byte-for-byteidentical 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.
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:
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
ChangedOutputIsNeverElidedpins this, including the harder direction: after content changes, thenew 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_filerepeated five times across a session costs 40 KiB once instead of five times, and thecontext window it would have consumed is what pulls compaction forward.
What the model sees
Worded to say explicitly that the tool ran, so the model does not treat it as a cache hit of
unknown age.
Scope decisions
session_endRead-only-ness comes from the tool's own
ReadOnlyHintannotation rather than a hard-coded namelist, so it works for MCP tools that declare it too (
tools.ToolAnnotationsismcp.ToolAnnotations).Implementation
Three small pieces, all at existing seams:
pkg/hooks/types.go— newInput.ToolReadOnlyfield mirroring the tool'sReadOnlyHint. Itis
falsewhenever the hint is absent or the tool is unknown to the agent, which is the fail-safedirection: a consumer keyed on read-only-ness stays inert rather than guessing.
pkg/runtime/toolexec/dispatcher.go— one line populating it inapplyToolResponseTransform, wherec.toolis already in hand.pkg/hooks/builtins/elide_repeated_tool_results.go— the builtin. Dispatches on event so asingle name covers both legs, the same pattern
redact_secretsuses. State is a package-levelper-session map of
(tool, args) → sha256(last output), mutex-guarded because parallel tool callsdispatch hooks concurrently. Keys are
sha256(toolName ‖ 0x00 ‖ json.Marshal(args));encoding/jsonsorts 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 directionsFirstCallPassesThrough,IdenticalRepeatIsElided— the happy pathNonReadOnlyToolIsNeverElided,ErrorResultIsNeverElided— the scope rulesDifferentArgsAreDistinct,ArgOrderIsIrrelevant— key correctnessSessionsAreIsolated,SessionEndForgetsState,PerSessionKeyCapIsBounded— state lifecycleConcurrentDispatch— runs under-raceIsRegistered,UnsupportedEventIsNoOp,NilInput,SmallPayloadNotWorthElidingThese tests deliberately do not use
t.Parallel(): they share the package-level store, which isthe 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.go test ./pkg/hooks/...go test ./pkg/runtime/...go test -race -count=1 ./pkg/hooks/...golangci-lint run ./pkg/hooks/... ./pkg/runtime/toolexec/...(v2.12.2, CI's pin)go run ./lint .gofmt -l,go build ./...go test ./...pkg/teamloaderfails — pre-existing (Google Cloud ADC), unrelatedNot in this PR
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.
decision once behaviour is measured.
feature with a real invalidation problem to solve, and should be argued separately.