fix(oauth): keep the ChatGPT chain alive when the client echoes tool defaults - #214
Conversation
bman654
left a comment
There was a problem hiding this comment.
This is a real bug and the fix is the right shape — thanks for finding it, and for staging the tests through response.output_item.done rather than planting head state. We confirmed the input actually reaches production: a real 2.1.267 request carries Edit.input_schema.properties.replace_all.default === false, the real client echo adds replace_all: false to an Edit the model emitted without it, and the AI SDK serializer preserves that default into the Responses tools payload. So this isn't a fix whose tests manufacture a signal that never arrives. One structural change is needed before merge, and the root-cause story needs correcting.
Must fix: the defaults map cannot be process-global
toolSchemaDefaults is a module-level map keyed only by tool name, last-writer-wins across every client, partition and session a clodex server serves, and a schema with no defaults never clears a same-name entry. Two consequences, both reproduced through the real WebSocket transport:
(a) A regression on an ordinary flow — not "pre-fix behaviour". continuationMatch caches the head side permanently (entry.canonicalPrefix ??=) on the first scan that touches the head, under whatever the map holds at that instant; the client side is recomputed per request. So: client A's turn emits replace_all: false explicitly → client B on the same server declares a same-named tool with default: true → A's Explore subagent (same session, same partition, no Edit in its tool list so nothing re-records A's schema) scans A's idle head and caches it with replace_all kept → A's next main turn strips it on the client side → history_mismatch_new_head, full resend. Pre-fix both sides were byte-equal and continued. That's the everyday "parent edits, then delegates to Explore" sequence, and the cache never recomputes for the head's lifetime. The same asymmetry opens within one request across the pacer.admit() wait, no subagent needed.
(b) Over-strip. With B's stale default: true and A declaring no default, A's emitted {"replace_all": true} and a client echo of {} compare equal → continuation on previous_response_id, omitting a genuine history change. Reaching a wrong upstream memory this way needs two contrivances (a same-named tool from another client with a default equal to the emitted value, and the current client rewriting the echo via a PreToolUse hook), and call_id uniqueness means two different responses can never collapse — so this one is low-severity. But it's silent, and the comment's claim that a stale entry "can at worst leave a property unstripped" is wrong in both directions and needs to go, along with the matching sentence in the doc.
The fix has a precedent in this file: headRequiredToolProps snapshots required from the head's own turn precisely because — quoting the existing comment at the canary — "reading the current turn's tools instead lets an unrelated schema change flip the verdict in either direction." Do the same here: make recordToolSchemaDefaults a pure per-request function (mirror requiredToolProps(payload)), thread the map through normalizeToolCallJson → stripSchemaDefaults, delete the global and its clear() in the test reset, and key entry.canonicalPrefix / canonicalEchoablePrefix on a fingerprint of that map so both sides are always normalized under the same snapshot. Within one client the fingerprint is constant, so the cache still works. Please add the two-client regression without a reset between clients — the beforeEach reset is what masks this today.
Trade-off to state: a request whose tools omit a tool that appears in its history gets no stripping for that tool (pre-fix behaviour). Subagent histories never contain parent calls, so the residual is narrow.
The root cause is the permission path, not Claude Code 2.1.268
The description, commit message, code comment, test comment and doc section all attribute this to a 2.1.268 change. It isn't one. replace_all carries .default(!1) in the 2.1.267 bundle byte-for-byte as in 2.1.268, and the ledger we mine locally shows the identical gap signature on 2.1.267 traffic. Captured echoes from real binaries against a synthetic server:
| binary | mode | echoed replace_all |
|---|---|---|
| 2.1.267 | --dangerously-skip-permissions |
absent |
| 2.1.268 | --dangerously-skip-permissions |
absent |
| 2.1.267 | acceptEdits |
false filled |
| 2.1.268 | acceptEdits |
false filled |
| 2.1.268 | default mode + --allowedTools |
false filled |
Bypass mode never fills, in either version. The permission path (checkPermissions → updatedInput written back to the transcript) fills, and did so before 2.1.268. So "not verified whether earlier versions fill defaults" is now verified — they do — and the release note should say when a tool call goes through the permission path rather than name a version. (Three of your open PRs pin 2.1.268 as the cause; we think that's because it's what you had installed, not carelessness — but the summary line is the changelog, so it matters.)
Scope notes
- Bash is out of reach for this change, and it's 77 of the 254 gap records in our ledger. The real Bash schema declares zero defaults across
command,timeout,description,run_in_background,dangerouslyDisableSandbox. A 2.1.267 probe shows a different mechanism — scalar type coercion:"timeout":"5000"echoes as5000,"run_in_background":"false"asfalse— which would produce the sameequalAfterStrip: false. Not provable against the hash-only ledger for those records, but it's the leading follow-up, and worth its own issue rather than folding in here. - "Every Edit" outruns the corpus: the 177 Edit diagnostics collapse to 8 distinct expected/actual hash pairs; most records are repeats or losing-head diagnostics.
- Value on current
mainis real but smaller than the description's headline: of 254 gap records, 190 continued anyway (the gap was on a non-selected candidate) and 8 opened a new head. The 56 that wereparallel_isolatedcollapse to 3 distinct initiating misses, and #219 already stops the cascade after each — so this saves those 3 plus the 8, not 56 resends. - Namespace recursion is claimed but untested — one namespaced case, or narrow the claim.
Verified: merges cleanly with main (2567 tests green on the merge tree); feature deletion reds exactly the positive test; widening the strip reds exactly the negative.
…defaults
When a tool call goes through Claude Code's permission path, the echoed call
comes back with its zod defaults filled in: an Edit the model emitted without
`replace_all` returns as `replace_all: false`. The head snapshot holds the
model's raw arguments, so the strict-prefix comparison failed on every such
call and the whole conversation was re-sent uncached (measured 2026-09-11 on
one GPT-5.6 Luna session through the proxy: 77.8% → 92.1% cached input).
This is the permission path, not a Claude Code version. `replace_all` carries
the same `.default(false)` in 2.1.267 as in 2.1.268, and captured echoes from
both binaries agree: bypass mode never fills the property, `acceptEdits` and
`--allowedTools` fill it in both versions. `checkPermissions` writing
`updatedInput` back to the transcript is what fills it.
`toolSchemaDefaults(payload)` derives `{tool → {property → canonical default}}`
from ONE request's `tools` array, namespaced groups included, and
`normalizeToolCallJson` drops from BOTH sides of the comparison any `arguments`
property whose value equals its declared default. Compare-only: outgoing
payloads are untouched. A value that differs from the default, or a property
with no declared default, still diverges as before.
The map is per-request and pure, for the same reason `headRequiredToolProps`
snapshots `required` from the head's own turn. A process-global map keyed only
by tool name is last-writer-wins across every client, partition and session one
`clodex server` handles, and `entry.canonicalPrefix` caches the head side
permanently under whatever the map held when it was first built — so another
client's schema can flip the verdict in either direction: under-stripping loses
a chain that should have continued, over-stripping continues on a history that
genuinely changed. The two prefix memos and the in-flight `canonicalInput` memo
are therefore keyed on a fingerprint of the map they were built under and
recomputed when it changes; within one client that fingerprint is constant, so
the memos still do their job.
Trade-off: a request whose `tools` omit a tool appearing in its own history gets
no stripping for that tool, which is the pre-fix behaviour. Subagent histories
never contain the parent's calls, so the residual is narrow.
Tests stage heads through `response.output_item.done`: the continuation with an
echoed default, a new chain on a non-default value, a new chain when the schema
declares no default, a namespaced tool group, and a three-request two-client
regression with no reset between clients — one client's schema, a scan that
caches the victim's prefix under it, and the victim's next turn. Each reds under
the process-global behaviour or without its own fix.
Session: a5ae056f-79a5-4beb-831d-c16d7b2d37dc
803dadb to
20cc935
Compare
|
Pushed, rebased onto 2.12.1. The defaults map is per-request now, the attribution is corrected everywhere, and the two-client regression is in. The map is pure and per-request. The regression test follows your reproduction, and needed all three steps. My first attempt used two clients only and passed even with a global map, because the victim's own next request re-records its schema before the comparison — so the corruption only bites through the cached prefix, exactly as you said. The test is now: A establishes a head whose call carries Namespace recursion now has its own test, and it reds when the The attribution is fixed in the commit message, the code comment, both test comments, the doc and the PR description, which now carries your table. It was the version we had installed rather than a comparison — the corrected statement is that the permission path fills defaults ( The wrong comment is gone — the "can at worst leave a property unstripped" sentence, in the code and in the doc — replaced by what the two failure directions actually are. The doc also states the trade-off: a request whose Bash I left alone, per your note. Worth its own issue: the mechanism there is scalar coercion, not defaults, so it needs a different comparison rule and its own evidence. Suite on the rebase: 2634 tests, 116 files, green. One caveat on that number — the first full run had |
…change Normalize head diagnostics with the request's defaults so accepted continuations no longer record misleading tool-argument gaps. Use collision-safe defaults fingerprints and add transport tests that mutation-pin the omitted-reasoning and in-flight memo invalidations.
bman654
left a comment
There was a problem hiding this comment.
Thanks for this round. The must-fix is done the way we asked, and done carefully. The defaults map is now a pure per-request function. The global and its clear() in the test reset are gone. All three memos (canonicalPrefix, canonicalEchoablePrefix, and the in-flight canonicalInput) are keyed on a fingerprint of the map they were built under. The two-client regression runs without a reset. We checked that it actually catches the bug:
- Making the map process-global with a constant fingerprint reds exactly what you said it would: the new two-client test and one original test ("does not strip a property the tool schema declares no default for").
- Deleting only the
canonicalPrefixinvalidation reds exactly the new test.
Your note that your first two-client attempt passed against a global map is what made us trust the three-step shape.
The rest is closed too:
- The attribution to the permission path is correct in the commit, code comment, tests, doc and description.
- The false "can at worst leave a property unstripped" sentence is gone.
- The trade-off for tools a request omits is stated.
- The value claim is corrected to 3 + 8.
- "Every Edit" is narrowed.
- Bash is scoped out with the right reason.
- The namespace branch has its own test. Deleting the recursion reds it.
Thanks also for reporting the http-proxy-index timeout instead of just the green run. We saw the same file time out on a heavily loaded host. It passes in isolation at this head and on the base, the PR doesn't touch it, and CI is green. We agree it isn't this change.
Two things needed to change before merge. Both were small, so rather than send this back for a third round we made them ourselves in a follow-up commit on your branch (c795cb9), reviewed it with the same panel rigour, and are merging. Details below so you can see what changed and why.
1. The mismatch diagnostics compared without the defaults, so the canary fired on the continuations this fix rescues (fixed in c795cb9)
The per-request map reaches the matcher (canonicalItemStrings), but not the diagnostic path. These callers of normalizeToolCallJson still pass no map:
arraysEqual, used bycontinuationMismatchDetailsto findfirstMismatchconversationItemHash(expectedHash/actualHash)- the equality pre-check in
toolArgumentNormalizationGap mismatchDumpLine
In round 1 the map was global, so these got stripping for free. Making it per-request quietly took stripping away from them.
The result: the turn right after an Edit that went through the permission path continues correctly, as intended. But its ws_head_decision records this on the selected head:
decision= continuation
{"firstMismatch":1,"expectedKind":"function_call","actualKind":"function_call","expectedHash":"f6eed0eb1897dced","actualHash":"def701f5a5e6da3d","toolArgumentNormalizationGap":{"tool":"Edit","equalAfterStrip":false}}
That is the same signature the bug produced. Only decision differs. We ran the same probe against your round-1 head (803dadb) and got the normal "matched the whole prefix" shape (firstMismatch: 2, expectedKind: none, no gap).
There is no routing, caching, or terminal-visible regression; the selected head never warns. With --ws-diagnostics, though, the selected head records the misleading gap. But toolArgumentNormalizationGap is the canary that found this bug, and the ledger is how we measure fixes like this one. As it stands:
- every continuation this PR rescues keeps reporting the gap;
- a before/after count will not drop;
- the "continued anyway" bucket will grow.
It also means the Evidence bullet "toolArgumentNormalizationGap no longer reported" can't describe this head. The body reports a manual run, but nothing ties it to 20cc935 or says it was repeated after the per-request refactor; at this head the probe reproducibly records the gap.
Ask: normalize the diagnostic comparison under the same per-request map the matcher uses. Pass it (or toolSchemaDefaults(payload)) into arraysEqual, conversationItemHash, the toolArgumentNormalizationGap pre-check and mismatchDumpLine. We tried that as an experiment. It is about ten lines, it turns the probe back into the normal full-prefix shape, and responses-websocket plus both thinking test files stay green (165/165). That also means nothing committed pins this default-stripped continuation's diagnostic shape in either direction. Please add a test asserting that the continued head's diagnostic has no toolArgumentNormalizationGap after a default-stripped continuation. It reds on the current head. Then re-measure the manual bullet or drop it.
2. Two of the three memo invalidations had no test (added in c795cb9)
The two-client test pins the canonicalPrefix invalidation. The other two guards are right, but deleting either one leaves every committed test that imports responses-websocket.ts green. With both deleted it is 169/169 across responses-websocket, thinking-continuation, thinking-block-coalescing and claude-agent-id-context.
-
entry.canonicalEchoablePrefix = undefined. The two-client test has no reasoning item, so it never reaches the omitted-reasoning path. A test that does:- stages a head whose output includes a reasoning item and a call;
- lets a same-partition request build the memos under one defaults map;
- continues under a different map with the reasoning omitted.
That test reds on deletion: a valid omitted-reasoning continuation is lost and opens a new socket.
-
The
canonicalInputToolDefaultsIdblock. A test for this one:- keeps a turn in flight;
- lets one same-partition arrival build the in-flight memo under one map;
- sends a second arrival under a different map.
It flips the lineage-gate verdict on deletion. We staged it in both directions: a new head where
parallel_isolatedwas right, and isolation where a new head was right.
Your reply presents the in-flight guard alongside the mutation-checked one. That's fair, but it isn't pinned yet, and the repo's own gate asks for each independent guard to be mutated. We wrote both tests as probes against the real transport, with head state staged through response.output_item.done, so they are cheap to add.
Also folded into c795cb9
- Fingerprint serialization can collide.
toolSchemaDefaultsFingerprintjoins raw property and tool names with=,,,:and;. So{a: 1, b: 2}and a single property literally nameda=1,bwith default2both hashEdit:a=1,b=2. We reproduced both directions through the transport: a lost chain, and, with a client also rewriting the echo, a continued changed history. Switching toJSON.stringifyof the sorted[name, [[prop, value], …]]tuples turns both probes green. It needs two different schemas for the same tool name in one partition, with property names built to collide, and every defaulted property name we found in the 2.1.269 bundle is a plain identifier. It is a three-line change, so we made it, with a test. - The trigger described in the comment and doc predates the rebase. The doc's "everyday parent-then-subagent sequence", the matching source comment, and "a title generation, a subagent" in the test comment all describe what we reproduced in round 1. That was on a base before #215, and that framing came from our review, so the stale part is ours. Since #215 the partition key includes
x-claude-code-agent-id, so a subagent never scans the parent's heads. With the head guard removed, the two-client scenario reds when step 3 carries no agent id and stays green when it carries one. The live trigger now is a request in the same partition with a different tool list, such as a main-agent auxiliary request or a mid-session tool-list change. "Both were reproduced through the real WebSocket transport" should be read as "on the old base". The two-client test is still the right assertion. It also reds without client B, so what it really pins is same-partition fingerprint invalidation. - Stale evidence wording:
- Feature deletion now reds two tests: the positive one and the namespaced one.
- "Widening reds exactly the negative": widening to strip any declared-default property regardless of value reds the value-differs test, as claimed. Widening to strip a false-valued property with no declared default, inside a tool that has one, reds nothing. "does not strip a property the tool schema declares no default for" gives the tool zero defaults, so it returns before the per-property rule. A negative with
replace_alldefaulted and a second, undefaulted property in the echo would close that. - The test comment, commit body and description say "three requests" but number four steps.
- The summary line is the changelog entry. "keep the ChatGPT chain alive when the client echoes tool defaults" leans on internal words ("chain", "echoes", "tool defaults"). It doesn't say what a user notices: the whole conversation was being re-sent uncached. Something like
fix(oauth): stop resending the whole conversation uncached after an approved tool callwould read better. We set it at squash time.
What we verified vs took on trust
Verified at 20cc935, all with an isolated CLODEX_HOME and a dead ambient proxy:
- Typecheck and build pass.
- The full suite is green: 116 files, 2634 tests, matching your count. Earlier runs on a host at load ~50–100 hit timeouts, but only in files this PR doesn't touch.
responses-websocket.test.tspasses 155/155.- Every mutation result above was run on the full file, with the source restored from a snapshot afterwards.
- The diagnostic regression was A/B'd against
803dadb.
Taken on trust: the Luna session numbers (77.8% → 92.1%), and the ledger counts, which we produced in round 1 and you carried forward correctly.
What the follow-up commit contains
c795cb9, one commit on top of your 20cc935: the diagnostics path (arraysEqual, conversationItemHash, the gap pre-check, mismatchDumpLine, and the equalAfterStrip comparison itself) now normalizes under the same per-request map as the matcher; the fingerprint is JSON.stringify of sorted tuples; five new transport tests (rescued-continuation diagnostic shape, omitted-reasoning memo invalidation, in-flight memo invalidation, fingerprint collision, undefaulted false-valued property not stripped, compound null-filler + defaulted-echo gap) each red under exactly the mutation they target and nothing else; and the comments/doc are corrected as above. Verified at c795cb9 with an isolated CLODEX_HOME and a dead ambient proxy: typecheck, build, and the full suite (116 files, 2640 tests). An opus-family reviewer independent of the implementer attacked the commit and re-ran every mutation before we merged.
Thanks again for the find and for the careful round-2 work — the per-request map and the two-client test are exactly right, and they're what shipped.
What this changes for users
On a ChatGPT/Codex plan, a tool call that goes through Claude Code's permission path comes back with its schema defaults filled in, and clodex then lost its place in the conversation and re-sent the whole thing uncached on the next turn. Clodex now treats a declared default as filler on both sides of the comparison, so an
Editno longer breaks the chain. On one Luna session this took cached input from 77.8% to 92.1%.Problem and root cause
Reachable by any ChatGPT/Codex OAuth user whose model calls
Edit— or any tool whose schema declares a default — when the call goes through the permission path.checkPermissionswritesupdatedInputback to the transcript, so the echoed call carriesreplace_all: falsefor anEditthe model emitted without it. The stored head holds the model's raw arguments, socontinuationMatchsees afunction_callwith the samecall_idandnamethat compares unequal, and the request goes tohistory_mismatch_new_headwith full context.Corrected from the first version of this PR: this is not a Claude Code 2.1.268 change.
replace_allcarries the same.default(false)in the 2.1.267 bundle, and captured echoes from both binaries against a synthetic server agree:replace_all--dangerously-skip-permissions--dangerously-skip-permissionsacceptEditsfalsefilledacceptEditsfalsefilled--allowedToolsfalsefilledThe original attribution came from the version we happened to have installed, not from a comparison. Corrected in the commit message, the code comment, the test comments and the doc.
Measured 2026-09-11 on one GPT-5.6 Luna session through the proxy (a 14-turn bug-fix task): 2 turns fully uncached at ~88k tokens each, both directly after an Edit, with
--ws-diagnosticsreportingtoolArgumentNormalizationGap {tool: Edit, equalAfterStrip: false}for each. The tool-argument canary was doing its job; this is the gap it was pointing at.Value, corrected: of 254 gap records in our local ledger, 190 continued anyway (the gap was on a non-selected candidate) and 8 opened a new head; the 56
parallel_isolatedones collapse to 3 distinct initiating misses, and #219 already stops the cascade after each. So this saves those 3 plus the 8 — not 56 resends. "Every Edit" also outruns the corpus: the 177 Edit diagnostics collapse to 8 distinct expected/actual hash pairs.The change
toolSchemaDefaults(payload)derives{tool → {property → canonical default}}from one request'stoolsarray, namespaced groups included, andnormalizeToolCallJsondrops from both sides of the comparison anyargumentsproperty whose value equals its declared default. Compare-only: the outgoing payload is untouched. A value that differs from the default (replace_all: true) and a property with no declared default still diverge as before.Per-request, not process-global (the review's must-fix). A module-level map keyed only by tool name is last-writer-wins across every client, partition and session one
clodex serverhandles, andentry.canonicalPrefixcaches the head side permanently under whatever the map held when it was first built. So another client's schema could flip the verdict in either direction — under-stripping loses a chain that should have continued, over-stripping continues on a history that genuinely changed. Same precedent asheadRequiredToolPropssnapshottingrequiredfrom the head's own turn. The two prefix memos and the in-flightcanonicalInputmemo are now keyed on a fingerprint of the defaults map they were built under and recomputed when it changes; within one client that fingerprint is constant, so the memos still do their job. The comment claiming a stale entry "can at worst leave a property unstripped" is gone from both the code and the doc.Trade-off: a request whose
toolsomit a tool appearing in its own history gets no stripping for that tool, which is the pre-fix behaviour. Subagent histories never contain the parent's calls, so the residual is narrow.Left out: nothing is done for
custom_tool_callinputs (no schema to learn defaults from) or for nested-object defaults; neither has been observed. Bash is out of reach for this change — its real schema declares no defaults at all, and a 2.1.267 probe shows a different mechanism there (scalar type coercion:"timeout":"5000"echoes as5000), which deserves its own issue rather than being folded in here.Evidence
pnpm typecheck && pnpm test && pnpm buildon the rebase onto 2.12.1: 2634 tests, 116 files, isolatedCLODEX_HOME.replace_allexplicitly underdefault: false; client B runs a turn declaring the same tool withdefault: true; a further request in A's own partition declaring noEdittool scans A's idle head and caches its prefix; A's next real turn must still continue on its own head. It reds when the map is made process-global with a constant fingerprint — which also reds one of the original tests.stripSchemaDefaultsfromnormalizeToolCallJsonreds exactly the positive test; widening the strip reds exactly the negative.toolArgumentNormalizationGapno longer reported.Failure and rollback behavior
Runtime failure mode is the pre-fix one: a property not stripped, one more mismatch. Nothing persisted. Reverting the commit restores the old comparison.