Skip to content

🤖 fix: send each native tool_reference once per request - #5935

Merged
ibetitsmike merged 8 commits into
mainfrom
mike/tool-search-dedupe-refs
Oct 9, 2026
Merged

ibetitsmike merged 8 commits into
mainfrom
mike/tool-search-dedupe-refs

Conversation

@ibetitsmike

@ibetitsmike ibetitsmike commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

In Anthropic native tool-search mode, every tool_catalog_search result emits tool_reference blocks for all matched deferred tools, and Anthropic expands every reference into the full definition, repeats included (#5413). A later search re-matching an already-loaded tool re-paid its whole schema on every subsequent request (observed: ~2K tokens per repeat; +87 to +521 tokens per repeated tool in the #5413 grid). #5416 fixed the budget accounting only.

This adds dedupeNativeToolReferences, a pure, idempotent, request-only projection over the final per-request messages: repeats are dropped in transcript order. A fully repeated result becomes a deterministic "All matched tools are already loaded: …" text result; a mixed result drops only its repeats, since Anthropic rejects a tool result mixing text with references. Every projected part stashes its raw output under providerOptions.mux (adapters only serialize their own namespace, so it never reaches the wire), which makes the projection byte-exactly invertible for both shapes. Untouched messages keep their identity so exact-append budget anchors and the cached prefix stay intact. Persisted results keep their raw matches, so when compaction drops the first referencing result, a later result re-references the tool by construction. Scoped (activeTools) mode is unchanged; countToolReferences stays as the budget backstop.

Implementation

Applied wherever final per-request messages are produced, so live and replayed requests are byte-identical by construction: messagePipeline (after the orphan-strip transforms), StreamManager's prepareStep, and the replay request builder's per-step mirror. In prepareStep the dedupe runs after the continuous-compaction prefix swap, and swapPrefix itself restores stashed raw outputs in the retained tail, so every swap site (per-step and the consumed-swap fallback rebuild) re-decides references against the swapped transcript with no per-run restoration state.

Note: existing conversations pay a one-time cache-prefix miss after deploy (repeat references disappear from replayed history).

Validation

  • Red-green on the dedupe path, the swap ordering, the stash inversion (byte-exact round trip for full and mixed projections, per-occurrence under reused toolCallIds), and the no-mixing rule; each toggle proven failing on the prior implementation.
  • Live-vs-replay byte parity, parallel same-step searches, compaction re-reference, append-prefix stability, and the 🤖 tests: guard the prompt-cache prefix across state changes (#5254) #5296/🤖 tests: add a native deferred-activation case to the prompt-cache prefix matrix #5406 prefix matrix all covered by tests.
  • tsgo --noEmit, eslint, prettier clean; toolCatalog, streamManager (requestShaping, continuousCompaction, errorRecovery, contextBudget), aiService, messagePipeline, replayVerify/fixture/cacheAudit suites green. No live count_tokens harness exists in-repo; token effect not measured against the provider.

Delivery record

  • Remote UAT (Coder Agents on dogfood): PASS at 1e7f03c — chat. Verified on the wire: no duplicate reference in any of 27 native requests, repeat searches answered with the already-loaded text, tool callable throughout, restart replay clean, scoped mode unchanged; base-commit control reproduced the duplicate references. Later heads (5a3204c … c21bb2c = current) are Codex-review fixes not exercised live; the current head's repeat-search wire shape matches the UAT-observed already-loaded text.
  • UAT observations (low, not blocking): the repeat-search transcript card shows "1 match" with the description while the model received the already-loaded text; a mixed query carries no note that the repeated tool was already loaded.
  • Codex review record (all threads resolved with fix + evidence replies): r1 P1 swap-dropped projected anchor → 5a3204c; r2 P2×2 dedupe-before-orphan-strip, toolCallId-keyed restore → 55e524e; r3 P2×2 projection-map collisions, map lost across restarts → stateless invertible markers, fa97dc3; r4 P1+P2 markers mixed with retained references, fallback swap site → 7c96d3d; r5 P1×2 mixed projection not invertible, stale repeated-search assertion → raw-output stash replaces markers, 3847486; r6 P1×2+P2×2 (loop paused for a human decision, resumed on "fix all") requestShaping expectation missed the stash, exact-prose assertion, stash counted by the context-budget estimator, cache-control write clobbered part providerOptions → budget walker skips the stash key, cache options merge instead of replace (both red-green), structural assertions, c21bb2c.

Generated with xum • Model: anthropic:claude-fable-5 • Thinking: high • Cost: $121.28

A tool_catalog_search result referenced every matched deferred tool even
when an earlier result in the same request already loaded it, and
Anthropic expands every tool_reference into the full definition again
(#5413). dedupeNativeToolReferences is a pure, idempotent, request-only
projection over the final per-request messages (history pipeline +
per-step transforms + replay mirror): repeats are dropped in transcript
order, an all-repeat result becomes a deterministic text result, and
persisted results keep their raw matches so a compacted prefix that
dropped the first reference re-references later.
A continuous-compaction prefix swap replaces the prefix the dedupe
projection is decided against, and the SDK accumulates later step inputs
from prepareStep's returned messages, so deduping inside
transformStepMessages (pre-swap) permanently lost a retained tail's only
surviving reference. Move the per-step dedupe after the swap block (and
apply it to the rebuilt first step and the settled-step budget floor),
mirror it last in the replay builder, and guard the ordering with a
live-swap red-green test.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T11:52:36.073215Z c21bb2c New commits
🔒 Security Review ✅ Completed 2026-10-09T11:52:47.384742Z c21bb2c New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e7f03c181

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/streamManager.ts
The reference dedupe is a request-only projection, but the AI SDK
accumulates the next step's input from the returned (projected)
messages. A later prefix swap that dropped a reference's first
occurrence could not fall back to the retained duplicate because the
carried copy was already projected to text, unloading the tool for the
rest of the turn.

Keep the raw tool-search results seen by this run's steps in a per-run
map (first sighting is raw: results enter the step messages when their
tool executes and are only projected afterwards) and feed the swap from
the restored transcript; the existing post-swap dedupe re-projects
whatever is still a repeat.

Codex P1 on #5935.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a3204c92e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/messagePipeline.ts Outdated
Comment thread src/common/utils/tools/toolCatalog.ts Outdated
Round 2 Codex findings on the dedupe placement:

- Pipeline: dedupe ran before transformModelMessages, whose orphan strip
  (self-healing for interrupted/corrupted history) can remove a result
  whose reference consumed a tool's dedupe slot, unloading the tool from
  the final request. Dedupe now runs after the transforms, before cache
  control.

- Swap restore: the per-run raw-result store was keyed by toolCallId,
  which providers can reuse across turns and steps, so restoration could
  replace an unrelated retained result (the fixture's own bash result
  exhibits the collision). Dedupe now reports each projection it makes
  and the swap path restores by matching the serialized projected part,
  chain-compressed to the fully raw form.

Codex P2 x2 round 2 on #5935.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55e524e941

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/streamManager.ts Outdated
Comment thread src/node/services/streamManager.ts Outdated
Comment thread src/node/services/streamManager.ts Outdated
Round 3 Codex findings showed the per-run projection store leaks edge
cases by construction: byte-identical projections under a reused
toolCallId collide in the map, and fallback/retry streams allocate a
fresh closure that cannot restore projections carried from before the
restart.

Remove the store entirely. The dedupe now replaces each duplicate
tool_reference with a deterministic text marker naming the tool, and the
swap path inverts markers back to raw references with a pure function of
the messages (unprojectNativeToolReferences) before re-projecting
against the swapped transcript. Restoration is per occurrence and
stateless, so ID collisions and stream restarts cannot lose it.

Codex P2 x2 round 3 on #5935.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa97dc3872

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/common/utils/tools/toolCatalog.ts Outdated
Comment thread src/node/services/streamManager.ts Outdated
Anthropic rejects a tool result mixing text with tool_reference blocks,
so the invertible markers may only appear in fully repeated results
(text-only output). A mixed result drops its repeats instead; the raw
form stays recoverable from persisted history on the next turn.

Also move the marker unprojection into swapPrefix itself so every swap
site inverts identically - the consumed-swap fallback rebuild called
swapPrefix directly and would have left a retained marker without its
reference.

Codex P1+P2 round 4 on #5935.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7c96d3da34

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/common/utils/tools/toolCatalog.ts Outdated
Comment thread src/node/services/aiService.test.ts
… byte-exactly

The marker scheme could not invert a mixed result: the dropped repeat left
no trace beside the retained references (text may not mix with them), so a
prefix swap that dropped the repeat's anchor lost the reference. Stash the
raw output on the projected part under providerOptions.mux instead; markers
are gone, a fully repeated result returns to the deterministic already-loaded
text, and unprojectNativeToolReferences restores the stash byte-exactly for
both full and mixed projections.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3847486d7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/node/services/streamManager.requestShaping.test.ts Outdated
Comment thread src/common/utils/tools/toolCatalog.ts
Comment thread src/node/services/messagePipeline.ts
Comment thread src/common/utils/tools/toolCatalog.test.ts Outdated
@ibetitsmike

Copy link
Copy Markdown
Contributor Author

Review-loop checkpoint — pausing for human direction

Per the repo's early-stop rule (loop not converging), I'm pausing instead of pushing a round-7 fix. Findings per round: r1: 1, r2: 2, r3: 2, r4: 2, r5: 2, r6: 4 — all six rounds in the same dedupe/inversion area, across three successive inversion mechanisms (per-run restore map → text markers → raw-output stash). Each redesign fixed the prior class and spawned new subsystem interactions.

Round 6 findings, verified by execution:

  1. P1 requestShaping test (confirmed red) — per-step transform drops a native tool_reference... fails on head 3847486: it expects the old stash-free mixed projection. My miss: I didn't rerun this suite after the stash change. CI on the current head is red because of this. Mechanical fix ready.
  2. P2 budget counting (confirmed by code reading) — prepareAssembledRequestTokenCount serializes messages wholesale, so the stash (never on the wire) inflates preflight estimates; ~0.5–1K tokens per projected fully-repeated result. Real, bounded; fix = exclude the stash from budget serialization (more mechanism).
  3. P2 cache control clobbers the stash (confirmed by code reading) — addCacheControlToLastContentPart replaces the last part's entire providerOptions, so a fallback swap after cache application can't restore a projected result. Real; fix = merge instead of replace (more mechanism).
  4. P1 exact-prose assertion (style) — valid per AGENTS.md's generated-copy rule, but it directly contradicts r5, which required the identical hard-coded sentence to pass unchanged in aiService.test.ts (still there). Churn signal.

Options:

  • (a) Continue the loop: all four fixes are bounded (~40 lines); I apply them, push, take round 7. Risk: the stash keeps meeting new subsystems.
  • (b) Simplify the design: drop in-transcript invertibility entirely — e.g. have the swap path rebuild the retained tail's search outputs from persisted history (the replay path already recomputes raw outputs), so no stash/markers/map exists to invert. Larger change, needs design review.
  • (c) Reduce scope: dedupe only in the history pipeline (persisted prefix), skip per-step dedupe so swaps never interact with projections; keeps most token savings, deletes the whole inversion surface.

State: UAT PASS @1e7f03c181, r1–r5 threads all resolved with fixes, r6 threads open, CI red on head per finding 1. Goal paused; say the word and I resume with (a), (b), or (c).

…rites

- measureBudgetContent skips providerOptions.mux.rawToolSearchOutput on
  tool-result parts: the stash never reaches the wire, so it must not
  inflate assembled-request estimates toward the hard ceiling.
- addCacheControlToLastContentPart merges cache options into existing
  providerOptions instead of replacing them, so the restoration stash
  survives cache application on fallback paths.
- requestShaping mixed-result expectation now includes the stash.
- Projection tests assert structure (text output, dropped names, stash
  bytes, idempotence) instead of the exact generated sentence.
@ibetitsmike
ibetitsmike added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit 34ff385 Oct 9, 2026
34 of 35 checks passed
@ibetitsmike
ibetitsmike deleted the mike/tool-search-dedupe-refs branch October 9, 2026 12:13
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.

1 participant