Skip to content

feat(hooks): flag-gated same-batch read coalescer - #323

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
feat/read-coalescer
Aug 27, 2026
Merged

feat(hooks): flag-gated same-batch read coalescer#323
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
feat/read-coalescer

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Summary

Adds hooks-tool-dedupe, a new additive, default-OFF hook module that
coalesces byte-identical repeated read_file / load_skill results within a
single parallel tool batch of a single session.

Measured basis (5 gpt-5.6 runs; full spec: yoc-dedup-spec.md, Section 2):

Class Reads Share Share of dup tokens
Same LLM turn, same parallel batch 1 196 37% 87%
Cross-session (rejected as out of scope) 129 4% 12%
  • Content byte-identical in 100% of the 1,196 measured same-batch repeats
    (0 divergences) -- this is why the mechanism can be lossless: it coalesces
    an exact-content match, never guesses from a staleness heuristic.
  • Deduplicating just this class removes 16.4% of all input tokens across
    the measured runs, with zero information loss, because the first
    occurrence always carries the content and the marker for every duplicate
    is delivered in the same tool-result turn, adjacent to it.

Mechanism

A tool:post hook can already replace the result content the LLM sees, via
HookResult(action="modify", data={..., "result": <new str>}). Verified
against loop-streaming's parallel (_execute_tool_only) and sequential
tool-execution paths, both of which detect a hook-modified result by object
identity (returned_result is not result_data). hooks-tool-truncation
(amplifier-bundle-attractor) is the existing, exercised precedent for this
exact shape -- this hook is a sibling of it, registered at a lower priority
(5 vs 10) so truncation sees an already-short marker and no-ops.

Hard constraints honored

  • Object identity, never in-place mutation. The hook always returns a
    new dict with a new str under "result".
  • Sequential calls are never deduped. The sequential tool-call path never
    carries a parallel_group_id (None there), so "no batch id -> no
    dedupe" falls out of the payload shape, not a separate check.
  • Per-session, in-memory state only. One dict on the hook instance
    (parallel_group_id -> key -> holder entry), bounded by an
    insertion-ordered LRU (max_batches, default 8). No tree id, no
    cross-session storage, no cleanup path needed -- it dies with the session.
  • Exact-content invalidation, not a heuristic. A same-key sha256
    mismatch within a batch is delivered untouched and counted as
    dedupe:divergent (the canary for mid-batch file mutation; should be ~0).
  • Skill-load dedupe is zero incremental code. load_skill is in the
    default tools allowlist, so it dedupes via the exact same code path --
    not a separate mechanism, as the spec's Section 0 (row 4) argues.
  • Default-OFF. enabled: false in behaviors/agents.yaml; mounting this
    hook has zero runtime effect until a caller opts in. Not added to
    tasks.yaml (same ships-dark pattern as hooks-delegation-batching).

Telemetry

dedupe:coalesced (one per marker, carrying tool_name,
parallel_group_id, holder/duplicate call ids, target, bytes_saved,
sha256) and dedupe:divergent, both declared via
register_contributor("observability.events", ...) so the existing session
logging hooks auto-capture them -- no new instrumentation needed.

Note on the falsification metric: the spec's "re-read after marker"
guardrail (Section 9) -- the single most informative signal in its DTU
measurement plan -- is computed externally, post-hoc, by joining
dedupe:coalesced events against ordinary read_file tool:post events in
the following turn. It is intentionally not tracked as live in-hook
state: doing so would require remembering marked keys across a turn
boundary, which is exactly the cross-turn state this module's scope excludes
(spec Section 6.2, rejected outright). No new instrumentation is needed for
this measurement either -- dedupe:coalesced's existing target field is
sufficient for the external analysis to join against.

Testing

  • 25 new pure-function/hook-level tests in
    modules/hooks-tool-dedupe/tests/test_batch_dedupe.py, covering the
    spec's full test plan (Section 4.8): sequential-never-deduped, first-read
    passthrough, second-read marked, new-object/str-result identity checks,
    divergent-content passthrough, offset/limit key normalisation and
    isolation, min_bytes/failed-result/unknown-tool/malformed-input/
    missing-content passthroughs, load_skill dedupe, concurrent-batch
    single-content-holder (pins the no-await-between-lookup-and-store
    invariant), max_batches LRU eviction, disabled-is-total-noop, telemetry
    payload shape, and mount() wiring (priority=5, observability.events
    contributor, default-disabled).
  • Module suite: 25 passed.
  • Repo suite: 1634 passed, 1 skipped.
  • python_check (ruff-lint, ruff-format, pyright, stub-check): clean.

Scope

Purely additive: one new module directory
(modules/hooks-tool-dedupe/) plus one behavior-YAML entry
(behaviors/agents.yaml). No edits to read.py, tool-skills,
tool-delegate, loop-streaming, amplifier-core, or any provider module.
bundle.md needs no change -- the hook arrives via the agents behavior it
already includes, matching how tool-delegate is wired.

Rollback is either flipping enabled: false (already the default) or
deleting the behaviors/agents.yaml entry.

This PR is Stage 1 only (per the spec's staged rollout, Section 10):
merge with enabled: false, zero runtime effect. Flipping the default and
running the DTU A/B (Section 9) is a follow-up, not part of this PR. Stage 2
(ACI windowed reads) is a separate repo/PR entirely.

Not merging this PR myself -- flagging for review.

🤖 Generated with Amplifier

Adds hooks-tool-dedupe, a new additive hook module that coalesces
byte-identical repeated read_file/load_skill results within a single
parallel tool batch of a single session. Ships default-OFF.

Measured basis (5 gpt-5.6 runs, yoc-dedup-spec.md Section 2): 1,196
same-batch duplicate read_file calls -- 37% of all reads, 87% of
duplicate tokens -- with content byte-identical in 100% of cases (0
divergences). Deduplicating just this class removes 16.4% of all input
tokens across the measured runs, with zero information loss, because
the first occurrence always carries the content and every duplicate's
marker is delivered in the same tool-result turn, adjacent to it.

Mechanism: a tool:post hook can already replace the result content the
LLM sees, via HookResult(action="modify", data={...,"result": <new
str>}) -- verified against loop-streaming's parallel
(_execute_tool_only) and sequential tool-execution paths, which detect
a hook-modified result by object identity (`is not result_data`).
hooks-tool-truncation (amplifier-bundle-attractor) is the existing,
exercised precedent for this exact shape.

Key properties, matching the spec's hard constraints:
- Detection is object identity: the hook always returns a new dict
  with a new str under "result", never mutates the input in place.
- The sequential tool-call path never carries a parallel_group_id, so
  it is never deduped -- no batch id, no dedupe, by construction. This
  is the entire safety invariant for the sequential path.
- State is one dict on the hook instance (parallel_group_id -> key ->
  holder entry), bounded by an insertion-ordered LRU (max_batches,
  default 8). No tree id, no cross-session storage, no cleanup path
  needed -- it dies with the session.
- Detection is exact sha256 comparison of content in hand, not a
  staleness heuristic: a same-key mismatch within a batch is delivered
  untouched and counted as dedupe:divergent (the canary for mid-batch
  file mutation).
- Skill-load dedupe is not a separate mechanism: "load_skill" is in the
  default tool allowlist, so it dedupes via the exact same code path.

Telemetry: dedupe:coalesced (one per marker emitted, carrying
tool_name, parallel_group_id, holder/duplicate call ids, target,
bytes_saved, sha256) and dedupe:divergent, both declared via
register_contributor("observability.events", ...) so the existing
session logging hooks auto-capture them -- no new instrumentation
needed. The spec's "re-read after marker" falsification metric
(Section 9) is computed externally, post-hoc, from dedupe:coalesced
joined against ordinary read_file tool:post events in the next turn;
it is intentionally NOT tracked as live in-hook state, because doing
so would require remembering marked keys across a turn boundary --
exactly the cross-turn state this module's scope excludes (spec
Section 6.2, rejected).

Wired via behaviors/agents.yaml (like tool-delegate) so every
sub-session in a delegation tree inherits it; bundle.md needs no
change. Config ships default-OFF (enabled: false) -- mounting this
hook has zero runtime effect until a caller opts in.

Tests: 25 new pure-function/hook-level tests in
modules/hooks-tool-dedupe/tests/test_batch_dedupe.py, covering the
spec's full test plan (Section 4.8) -- sequential-never-deduped,
first-read passthrough, second-read marked, new-object/str-result
identity checks, divergent-content passthrough, offset/limit key
normalisation and isolation, min_bytes/failed-result/unknown-tool/
malformed-input/missing-content passthroughs, load_skill dedupe,
concurrent-batch single-content-holder (pins the no-await invariant),
max_batches LRU eviction, disabled-is-total-noop, telemetry payload
shape, and mount() wiring (priority=5, observability.events
contributor, default-disabled).

Module suite: 25 passed. Repo suite: 1634 passed, 1 skipped.
python_check: clean (ruff-lint, ruff-format, pyright, stub-check).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Admin-merge disclosure — self-authored PR, explicit user/operator direction

This PR is self-authored by the maintainer. Per the documented admin-merge pattern for this situation (self-approval is not possible; merge proceeds only under explicit operator direction — consistent with ~14 PRs merged this way this week), this will be merged via gh pr merge --admin once CI is green.

Basis for merge:

  • New module hooks-tool-dedupe: a same-batch read coalescer for duplicate tool reads within a single batch.
  • Measured basis: 1,196 measured same-batch duplicate reads across 5 eval runs; those duplicates account for 87% of duplicate-read tokens; content was byte-identical in 100% of measured cases; duplicate reads amplified input tokens by ~16.4%.
  • Ships dark: registered with enabled: false in the behaviors entry — zero behavior change until an operator opts in.
  • Test evidence: 25 module tests + 1634 repo tests green.
  • CI status: all required checks green at time of merge.

Merging squash, branch retained (--delete-branch=false).

@bkrabach
Brian Krabach (bkrabach) merged commit c179f5a into main Aug 27, 2026
7 checks passed
Brian Krabach (bkrabach) added a commit that referenced this pull request Aug 28, 2026
…idempotency section (evidence refuted) (#328)

A payload-level investigation found that three recent changes were built on
a measurement artifact: analyses counted the same events 2-4x across
duplicate snapshot copies of session files (proven by identical
tool_call_id + nanosecond timestamps across "duplicates"). Deduped, the
phenomena each change was built to address vanish. This reverts all three
on that evidence.

1. Remove hooks-dedupe module entirely (PR #323 as hooks-tool-dedupe,
   renamed in PR #326). Deleted modules/hooks-dedupe/ and its `hooks:`
   entry in behaviors/agents.yaml. Deduped, the "same-batch duplicate
   reads" it coalesces do not exist: 0 across 314 sessions / 10,320 reads.
   Measured real value was ~19k tokens (~$0.003) across 5 runs against a
   claimed 16.4% savings -- a ~4,800x overstatement.

   Kept: tests/test_hook_module_classification.py (added by #326). It
   guards a real amplifier-core loader fragility (name-based module-type
   guessing misclassifying a `hooks-*` module as `tool`) that is unrelated
   to whether hooks-dedupe itself exists. Removed only the hooks-dedupe
   specific references: the hardcoded
   `assert "hooks-dedupe" in HOOK_MODULE_IDS` and the
   `test_hooks_dedupe_passes_real_validation` end-to-end test (and its
   now-unused `ModuleValidationError` import). The general, dynamically
   parametrized `test_hook_module_classifies_as_hook` test is untouched
   and still covers every remaining `hooks-*` module.

2. Remove PR #320's "BATCH YOUR DELEGATIONS" guidance block from the
   delegate tool's description in
   modules/tool-delegate/amplifier_module_tool_delegate/__init__.py,
   restoring the original "- Launch multiple agents concurrently when
   tasks are independent" line it replaced. A clean same-commit 5v5 A/B
   measured the guidance as inert (treatment waves median 4 vs control
   median 3) -- the earlier apparent win was a version confound -- while
   it cost ~175 tokens on every single request.

   PR #320's two context-file edits (context/agents/multi-agent-patterns.md,
   context/agents/delegation-instructions.md) are untouched; only the
   scope named above is in this revert. PR #327's own deletions in this
   same string (the CRITICAL/ALWAYS/NEVER preamble and the "DEFAULT TO
   DELEGATION" line) are also untouched -- they stay removed.

3. Remove PR #317's "## Idempotency Discipline" section from
   agents/git-ops.md. Its evidence -- "git-ops re-ran identical clone x8 /
   ls-remote x7 under context truncation" -- is refuted: deduped session
   data shows 8 clones of 8 *different* repos, each cloned once, with
   per-repo ls-remote calls. The thrash it was written to prevent never
   happened.

Verified: full suite green (uv run pytest tests/ -q: 1640 passed, 1
skipped -- down from 1642 by exactly the two hooks-dedupe-specific test
instances removed in (1), both accounted for). python_check clean on all
touched files (only pre-existing, unrelated ruff warnings remain in
tool-delegate's __init__.py, none introduced by this change). Repo-wide
grep confirms modules/hooks-dedupe/ is gone with no dangling functional
references (three remaining mentions of hooks-tool-dedupe/hooks-dedupe are
historical narrative inside the kept regression test's docstring/assert
message, explaining why that general test exists).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

CORRECTION: this module's premise has been refuted by payload-level validation. The "1,196 same-batch duplicate reads / 16.4% amplified tokens" figures were a measurement artifact — duplicate snapshot copies of session files counted the same events 2-4x (identical tool_call_id + nanosecond timestamps prove it); deduped, same-batch duplicate excess is 0 across 314 sessions / 10,320 reads. Additionally the openai provider chains server-side (deltas only on the wire), so the assumed re-send amplification does not exist; measured real value ≈ 19k tokens (~$0.003) across 5 runs. Module removed in #328; the loader-classification regression test from #326 is retained (it guards a real core loader fragility). The verified successor lever is windowed reads targeting first-read volume (39.1% of billed input) — tracked separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants