Make tool-set churn visible between turns - #980
TheGreatAxios wants to merge 1 commit into
Conversation
ebf31b0 to
1f6c45e
Compare
The tools array is the head of the provider's cached prompt prefix, so any change to it re-prefills the whole request. Whether that actually happens in real sessions is unmeasured: it shows up only as a billing and latency spike a turn later, with nothing tying it back to a mount. Log a digest when the set changes. Hashed, not verbatim — MCP tool descriptions are arbitrary-length server-supplied text and do not belong in the log stream. This replaces an earlier attempt that sorted the array by name. That was wrong: advertisedTools already orders deterministically, so sorting added no stability, and an alphabetical insert can land at index 0 and invalidate more of the prefix than appending does. A gate run carrying it measured cache-hit rate down 3-8 points across all four eval tiers. CL-7868
1f6c45e to
361aa3f
Compare
|
Closing. This PR was built on a premise that turned out to be wrong, and what survived review does not earn a merge. The original change sorted the tools array, on the theory that discovery order (MCP connect order, plugin load order, map iteration) reaches the wire. It does not: What remained after the rewrite was a debug log measuring whether tool-set churn happens at all. Nobody reported that symptom; it came out of my own cache probes, and the existing design is specifically built to prevent it. Landing an instrument to hunt for a problem the code already avoids is the wrong order — if the telemetry is wanted later it is a small change at that point. CL-7868 stays open and has been rewritten around the design with a real payoff: Branch left in place; nothing merged. |
Closes part of CL-7868.
Summary
This PR was rewritten after review. It no longer sorts the tools array.
The tools array is the head of the provider's cached prompt prefix, so any change to it re-prefills the whole request. Measured on OpenCode Go Responses: a warm session holds 99.3% cached, and a mount drops the next turn to 2–4%.
Whether that actually happens in real Corbits sessions is unmeasured. It surfaces only as a billing and latency spike a turn later, with nothing tying it back to a mount. This adds the digest so it can be counted.
Hashed rather than logged verbatim — MCP tool descriptions are arbitrary-length, server-supplied text and do not belong in the log stream.
What was removed, and why
The first version sorted the array by name, on the premise that discovery order (MCP connect order, plugin load order, map iteration) reaches the wire. That premise was wrong.
advertisedTools(src/agent/tool-search.ts:143) already projects onto a fixed built-in prefix plus activation order, and it documents that contract explicitly. Every director's tools come throughcomputeAdvertised, so connect order never reaches the wire.Worse, sorting is actively harmful.
toolsis serialized ahead of the system prompt, so the cache keeps everything before the first changed byte:Appending puts the invalidation point at the end of the tool block. An alphabetical insert can put it at index 0. Sorting can only move the damage earlier — it cannot save the system prompt either way.
A gate run carrying the sort measured cache-hit rate down 3–8 points in all four eval tiers, which is consistent with that reading. I reported that regression as unattributable at the time; this is the likely mechanism.
Also dropped:
localeCompareas the comparator, which is ICU- and locale-dependent — a non-deterministic comparator inside a byte-stability fix.The
src/director.test.tsassertion that a new tool lands at the end of the array is restored, since that behavior is intact and worth protecting.What replaces it
CL-7868 now describes the design that actually works —
tool_searchreturning schemas into the conversation tail instead of promoting into the array, a stable dispatcher inCORE_TOOL_NAMES, and folding used tools into the real array at compaction, where the re-prefill is already paid for. This PR is the observability step that sizes whether that is worth building.Testing
bun test src/director.test.ts59 pass,bun run typecheck,bun run lintclean.