Repository navigation
fix(core): shorten compound tool-call ids on the generate fallback - #144
pierreraby wants to merge 2 commits into
Conversation
Hosts store Responses tool-call ids compounded as <call_id>|<item_id> (73 characters for the observed call_<32hex>|fc_<32hex> shape). The /alpha/generate backend rejects call_id values longer than 64 characters with 400 input[N].call_id, so the fallback maps every referenced id to a deterministic per-request wire form shared by calls and results: pairing is preserved, distinct ids never collide, and short ids pass through unchanged. The Provider API path is untouched. Closes #141
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Memory benchmarkCompared base
Interpretation: Differences larger than their measured MAD: Physical footprint +0.9 MiB ± 0.8 MiB, Physical peak +0.9 MiB ± 0.8 MiB. Extension overhead above pi baseline
Values are medians of 6 alternating, paired runs. The value after Environment: pi
|
karaaslanz
left a comment
karaaslanz
left a comment
There was a problem hiding this comment.
Thanks for fixing #141 and adding call/result pairing coverage. I found one deterministic edge case in the exact PR head (b9a7233):
When an existing <=64-character tool-call ID equals the shortened candidate generated for an earlier overlong ID, generateToolCallIdMap can allocate that candidate to the long ID first. The existing short ID is then renamed, contradicting the documented guarantee that short IDs pass through unchanged. The outcome depends on message ordering.
Requested change: Reserve all pre-existing <=64-character IDs before allocating shortened wire IDs for long IDs; use the existing hash-suffix fallback for long-ID collisions. Add a regression with both orderings of the same colliding pair, asserting (1) the original short ID stays unchanged, (2) all distinct wire IDs remain distinct and <=64 characters, and (3) tool-call/result references still match.
I validated this bounded approach against the PR head in a local-only patch: 329/329 unit tests, TypeScript typecheck, Prettier, and 250 additional seeded shuffled-ID scenarios passed. The patch has not been pushed; these results are not upstream CI results for the patch.
Requesting changes until this short-ID pass-through edge case is handled.
Reserve every pre-existing generate tool-call id within the 64-character limit before allocating shortened wire ids. Long-id collisions keep using the hash-suffix fallback, so short ids remain unchanged regardless of message order. Add both-order regressions covering short-id preservation, distinct bounded wire ids, and call/result pairing. Addresses the edge case reported by @karaaslanz on PR #144.
|
Thanks Ömer — good catch. I independently reproduced it on Fixed in Local validation: 330/330 unit tests; full @karaaslanz could you take another look when you have a moment? Thanks for the thorough review! |
|
Hi @karaaslanz, when you have a moment, could you re-review the collision fix in We’d like to include this fix alongside #145 and #148 in the next release. Thanks again for catching that edge case! |
This branch has not been deployed
2 participants
Closes #141.
Hosts store Responses tool-call ids compounded as
<call_id>|<item_id>(73 characters for the observedcall_<32hex>|fc_<32hex>shape). The/alpha/generatebackend rejectscall_idvalues longer than 64 characters with400 input[N].call_id, so the fallback maps every referenced id to a deterministic per-request wire form shared by calls and their results: pairing is preserved, distinct ids never collide, and short ids pass through unchanged. The Provider API path is untouched.Verified: reporter's session replays to max 64 (was 73), 0 raw ids, pairing intact; unit suite green (pure 68/68 incl. 5 new, stream 42/42); typecheck + prettier clean.