Repository navigation
fix(chat): preserve reasoning_details that arrive after the reasoning block closes - #608
LukasParke wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if ( | ||
| details && | ||
| details.length > 0 && | ||
| details.length >= (mostComplete?.length ?? 0) | ||
| ) { | ||
| mostComplete = details; |
There was a problem hiding this comment.
🔴 Earlier reasoning blocks disappear on replay
When doGenerate returns multiple reasoning parts without tool calls, findMostCompleteReasoningDetails keeps only the last part on a tie. Earlier signed thinking blocks disappear from the next request.
Learn more
Non-streaming responses from doGenerate give each reasoning part a one-element reasoning_details array, unlike streaming's cumulative snapshots. The converter now replaces an earlier equally sized array with the last one. Thus it discards reasoning that the earlier implementation retained, and cannot reconstruct the whole response by choosing one carrier.
Example: A response has two signed Anthropic thinking details, First and Second, and no tool call. doGenerate returns [First] and [Second] on separate reasoning parts. The next request now sends only [Second], not both blocks.
Recommended fix: Preserve the distinction between cumulative streamed carriers and per-detail non-streamed parts. Aggregate distinct non-streamed reasoning parts in order, while selecting a single cumulative snapshot for streamed parts and tool calls; test both paths with multiple signed blocks and mixed summary/encrypted details.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch. Fixed in the latest commit: on an equal-length tie, a later carrier now wins only if it continues the current one entry by entry (an in-place merge such as a late signature). So separate per-detail doGenerate parts like [First], [Second] keep [First], as on main, and there's a test for it. Combining per-detail doGenerate parts is in scope for #600: its first reasoning part carries the full array, and 'most entries wins' selects it.
| /** | ||
| * Reasoning details that arrive after the visible reasoning block has closed | ||
| * (e.g. a late `reasoning.encrypted` blob from OpenAI or a signature-only | ||
| * delta from Anthropic) must still be replayed on the next turn, exactly once, | ||
| * when the stream is consumed over a real wire (chunks serialized as they | ||
| * arrive) rather than in-process. | ||
| */ |
There was a problem hiding this comment.
🟡 Issue regression has no E2E test
The late-signature regression lives under src/tests, but the contributor rules require issue fixes to add a regression under e2e/issues. The issue-specific suite cannot exercise this case.
Learn more
Contributing guidelines require regression tests for reproducible GitHub issues in e2e/issues/, named for the issue and using the prescribed structure. The new mocked round-trip test only runs in the Node and Edge unit suites. Existing issue #423 E2E tests do not reproduce a late detail across an actual serialized UI stream.
Example: Running pnpm test:issues does not include this late-signature and late-encrypted round-trip test because it is located in src/tests/.
Recommended fix: Add an issue-numbered test in e2e/issues/ that covers this late-detail reproduction against the applicable live model, retaining the mocked round-trip tests for deterministic coverage.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This bug was reported by a reviewer and has no GitHub issue to attach an e2e/issues/ regression to. The late-delta timing (a reasoning detail arriving after text starts) can't be triggered deterministically against the real API, so the deterministic wire-timing test in src/tests/ covers it instead.
An empty reasoning part that carries late reasoning_details (#608) is allowed, so the signature-only scenario now passes whatever order the PRs merge in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Behaviour is unchanged; only where the stripping happens moves, so this PR merges cleanly with #605 and #608 in either order: - chat: drop reasoningReplay where openrouterOptions is read, leaving the cacheControl destructure as on main (#605 extends that destructure). - completion: drop it from the assembled args in a separate statement, leaving the openrouterOptions line as on main (#605 rewrites it). - convert: put a blank line between the candidate selection and the replay filter so the hunk does not touch the findFirstReasoningDetails call (#608 renames it). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… block closes Snapshot reasoning_details on each carrier part (reasoning-end, first tool call) and, at flush, emit an empty reasoning part carrying the complete array when details arrived after the last carrier. The converter replays the most complete carrier so each detail is sent back exactly once. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nues the current one Keeps the first of separate per-detail reasoning parts (doGenerate) instead of replacing it with the last. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… tests #590 also appends a describe at the end of this test file, so the two appends conflicted. Placing this block after the multi-turn reasoning_details deduplication tests keeps both PRs mergeable in either order. Test content is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b1d7f32 to
2c34fcf
Compare
Behaviour is unchanged; only where the stripping happens moves, so this PR merges cleanly with #605 and #608 in either order: - chat: drop reasoningReplay where openrouterOptions is read, leaving the cacheControl destructure as on main (#605 extends that destructure). - completion: drop it from the assembled args in a separate statement, leaving the openrouterOptions line as on main (#605 rewrites it). - convert: put a blank line between the candidate selection and the replay filter so the hunk does not touch the findFirstReasoningDetails call (#608 renames it). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Description
Fixes a bug that was already on
main. IndoStream, the visible reasoning block is closed (reasoning-end) as soon as text starts. Anyreasoning_detailsthat arrive after that point were added toaccumulatedReasoningDetails, but no later persisted part carried them. Examples are a latereasoning.encryptedblob from OpenAI and a signature-only delta from Anthropic.reasoning-end(and the firsttool-call) held the same mutable array object, so later mutation showed through.reasoning-endright away, so the late blob or signature was lost, and the next turn replayed[summary]only. For Anthropic it replayed[], because the unsigned text gets stripped.Approach
This follows direction (a) from the issue.
reasoning-endand the firsttool-callnow get a copy of the accumulated details (each entry shallow-copied, because merges mutate entries in place). AreasoningDetailsChangedSinceSnapshotflag records whether details changed after the last snapshot.reasoningId !== undefined || reasoningDetailsAttachedToToolCall) and details changed since, flush emits an emptyreasoning-start/reasoning-endpair with the complete array. It has no deltas, so no second visible reasoning block appears.findFirstReasoningDetails(first tool call, otherwise first reasoning part) is replaced byfindMostCompleteReasoningDetails. The carrier with the most entries wins. On a tie, a later carrier wins only if it continues the current one entry by entry (same type, text or summary extended, or a signature added, or the same encrypted data), which is what a late signature merged in place looks like. Each streamed carrier is a cumulative snapshot, so this replays every detail exactly once. Separate per-detail reasoning parts fromdoGenerate([First],[Second]) keep the first one, as onmain. Carrying the full array there is what fix(chat): preserve encrypted-only reasoning across turns #600 does.I chose "last / most complete" over merging all carriers with
ReasoningDetailsDuplicateTracker. A merge would replay the stale unsigned copy of a text block: the tracker keys ontext, so the first, unsigned copy wins. It would also duplicate text or summary entries that grew after the snapshot, because their keys differ.Existing behaviour that still holds:
undefinedkeys are written (fix: prevent assigning undefined keys during streaming reasoning accumulation (#540) #547). The accumulation logic is unchanged, and snapshots are spreads of existing entries.Tests
src/tests/late-reasoning-details-roundtrip.test.tsserves turn 1 viaTestResponseControllerwith 20 ms gaps between SSE chunks. It goes throughstreamText→toUIMessageStream(each chunk JSON round-tripped, as on a wire) →readUIMessageStream→convertToModelMessages→ turn 2, and asserts the turn-2reasoning_detailswithtoStrictEqualplus an exactly-once count per detail. Four scenarios: late encrypted after text, late signature-only after text, late encrypted after a tool call, and late encrypted after text plus a tool call. The same scenarios also run throughresult.response.messages, JSON-persisted.origin/main, all 4 UI-path cases fail. Theresponse.messagescases pass onmainbecause of the in-process aliasing. They act as a guard here: with snapshots but without the trailing carrier they fail.e2e/issues/test: this bug was reported by a reviewer and has no GitHub issue, and the real API can't reproduce the late-delta timing deterministically. The wire-timing repro above covers it.doStreamtests asserted exactly onereasoning-start/reasoning-end. They now assert one visible block plus a trailing empty carrier with the complete details.should have correct reasoning-end metadata (pre-signature)…relied on the aliasing: its comment said the late signature "IS visible" inreasoning-end. It now asserts thatreasoning-endis a pre-signature snapshot and that the trailing carrier holds the signed detail.Composition with #600
I trial-merged
origin/claude/preserve-encrypted-reasoninginto this branch locally. It auto-merged with no conflicts. The two flush branches don't overlap: this PR handles the case where a carrier exists and details arrived late (reasoningId !== undefined || attached), and #600 handles the case where no carrier exists (reasoningId === undefined && !attached). With both applied,pnpm stylecheck,pnpm typecheckandpnpm testpass (587 tests). #600'sdoGeneratechange gives the first reasoning part the full array while later parts carry[detail]. "Most entries wins" picks the full array, so itsgenerateTexttests still pass. Optionally, #600's branch could usesnapshotReasoningDetails()instead of the live array. It doesn't need to, since that branch only runs at flush.Checklist
pnpm stylecheckandpnpm typecheckpnpm testand all tests passChangeset
pnpm changesetto create a changeset file (patch)Merge coordination
Merges cleanly with #590, #597 and #598 in either order, so no hand resolution is needed. The new
late reasoning_details carrier selectiondescribe now sits after the multi-turn reasoning_details dedup tests, not at the end ofconvert-to-openrouter-chat-messages.test.ts, which #590 also appends to. The test content is unchanged. I checked this with local integration merges of #590–#609 in both orders.🤖 Generated with Claude Code