Conversation
…ced them With Claude OAuth cloaking, the restore maps non-canonical MCP aliases (repeated server prefix, alias or semantic suffix, hybrid passthrough) and bare client names to the client's tool name, but the request-side remap writes back only the canonical alias. When the model wrote a non-canonical name, the replayed assistant turn then differs from the original response, which matters for a latest assistant turn that carries signed thinking. Record the produced name per caller and tool_use id when replaying the client's name would not reproduce it, and write it back on replay while the history block still carries the name the client was given, whatever the remap would write now. - Only callers with their own downstream API key get records, keyed by a hash of that key plus the tool_use id. Keyless callers share one alias secret, which cannot keep their records apart, so they get none. - Every response a keyed caller receives through Execute or ExecuteStream, cloaked or not, replaces or drops the record for each tool_use id it carries in one cache operation (the new BoundedLRU.Set), so a later response that reuses an id cannot inherit it. - Records hold owned copies of at most 512 bytes of id and names, and the least recently used of 10240 records is evicted first. This is best effort: after a restart, on another replica, once a record is evicted, or for a keyless caller, the replay writes the canonical alias, as before.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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.
Problem
With Claude OAuth cloaking, the proxy renames client tools to MCP aliases on the way up and restores the client's names on the way down.
Restore (
claudeMCPAliasResolver.resolveininternal/runtime/executor/claude_executor_request.go) accepts several non-canonical spellings of an alias and maps each one to the client name:A bare client name that the model writes passes through unchanged.
Replay: the request-side remap writes only the canonical alias back into the history's assistant messages, including the latest one.
So when the model itself writes a non-canonical name, the round trip loses the name it produced. The next request carries a different name in that assistant turn than the model's original response.
That matters for a latest assistant turn that carries signed thinking. We observed this error on such requests:
In the failing requests we inspected, the latest assistant message differed between the client's request and the request sent upstream only in
tool_use.name: the client name became the canonical alias. The thinking blocks and the text were byte-identical.Whether Anthropic's validation covers the
tool_usename is not proven, because the original upstream responses were not captured. Two other things could produce the same symptom, and this change doesn't address them:Reproducing
Synthetic (also the acceptance test). For an upstream
tool_use.nameN, the invariant is: restore(N) → client history → remap(history) must return N.A one-way check that remaps a body already containing a non-canonical MCP-shaped name is not enough. The remap leaves MCP-shaped names alone, while real client history holds the restored plain name.
Server side (derived, not run).
tool_usewhose name is a non-canonical alias the restore resolves, next to signed thinking.Change
Keep a small in-memory record of produced names.
ExecuteorExecuteStream(JSON and SSE) replaces or drops the record for eachtool_useid it carries.BoundedLRU.Setreplaces a value under the cache lock, so concurrent restores of one id give last-write semantics.HttpRequestpassthrough restores no tool names and doesn't drop records either.tool_useblock still carries the name the client was given, write the recorded name back. This holds whatever the remap would write now, so a tool-set change does not discard it.tool_useid, so one caller cannot read, replace or drop another caller's records. Keyless callers share one alias secret, which cannot keep their records apart, so they get none.This is best effort, not a guarantee. Records live in one process. The replay writes the canonical alias, which is today's behaviour, in these cases:
Because the bound is shared, heavy drift from one caller can evict another caller's records. Nothing changes for tool names that already round-trip.
Why this shape
The loss happens at restore time: several spellings collapse to one client name. So it cannot be fixed on the replay side alone.
Bashwhether it came from the canonical alias or was written bare. It would also turn drifted-but-resolvable tool calls into failed ones.A turn stays replayable for as long as the client keeps the conversation, across restarts and replicas. So no process-local record can make every replay exact. Exact replay in every case would need shared, durable state, such as the home KV the thinking-replay caches use. This change stays small and covers a live process, which is where a tool round trip normally completes.
Check
TestClaudeOAuthToolNameRoundTripKeepsProducedNamechecks the invariant: restore(N) → client history → remap(history) must yield N. It runs through stream and non-stream restore, and through batched and legacy replay.Record behaviour, each on all 4 paths unless noted:
TestClaudeProducedToolNameRecords:TestClaudeExecutorUncloakedResponseDropsProducedToolNameRecord: an uncloakedExecuteandExecuteStreamresponse of a keyed caller reuses a recorded id. The test reads the store before (record present) and after (record gone).TestResolveClaudeMCPAliasOptions: only a caller with its own API key is keyed.TestClaudeProducedToolNameRecordsConcurrentCallers: eight callers restore and replay one id concurrently, and each gets its own name.TestClaudeProducedToolNameRecordEviction:TestClaudeProducedToolNamesDoNotPinResponses: sixteen restores of 1 MiB responses must not keep them alive. Without owned copies, about 16 MiB stays live.TestBoundedLRUSetReplacesAndEvicts:Setreplaces a value in place, refreshes the key, and evicts the least recently used key when a new one crosses the bound.Each check fails when the property it guards is removed:
ExecuteandExecuteStreamseparately;Set.The eviction test fails when no replay refreshes the record. Without any record, the round-trip test fails exactly as dev does.
Also checked:
go test ./internal/runtime/executor/... ./internal/cache/...passes.-raceruns.go vetandgo build ./...are clean.-raceruns can failTestClaudeExecutorPrepareRequestAuthIsRaceFreeOnSharedCredential. It is a data race between the metadata write ininternal/auth/claude/identity.goand the read inclaude_executor_auth.go. It reproduces with that test alone on dev (-race -count=80: 3 failures).