You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(translator): hoist displaced tool results into their assistant turn - #18709
Clients that steer mid-turn (queued user messages, task notifications) can interleave a non-tool message between an assistant tool_calls message and part of its results:
[assistant(tc:c1,c2), tool(c1), user, tool(c2)]
Strict upstream validators reject the translated chat payload with a hard 400. Confirmed live against DeepSeek v4-flash (thinking) through the /v1/responses to chat downgrade, from a real session that ran on a Responses-native provider and then switched models:
[400]: An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. (insufficient tool messages following tool_calls message)
The shape beats both existing pair fixes:
fixMissingToolResponses collects the contiguous result run after the assistant message, so a result displaced past a non-tool message is never counted and a synthetic empty is inserted for its id.
The displaced real result still reaches the upstream, where it now also duplicates the synthetic id (DeepSeek: Duplicate value for 'tool_call_id').
stripOrphanedToolResults keeps it because its call id exists somewhere in the conversation.
Fix
New repairDisplacedToolResults pass, registered before the pair fixes in both translateRequest stages. For each assistant turn with tool calls it scans ahead (stopping at the next assistant message) and hoists any response messages that resolve the turn's remaining call ids back to the end of the turn's contiguous result run. Response carriers are matched through respondedToolIds, so both OpenAI role:"tool" messages and Claude-format tool_result user messages are handled.
After the pass, the displaced example above normalizes to:
[assistant(tc:c1,c2), tool(c1), tool(c2), user]
which every upstream accepts, with the real result content preserved instead of a synthetic empty.
Existing tests/unit/translator-toolcall-helper.test.ts passes unchanged (9/9) under node --experimental-strip-types --test (full runner env unavailable locally; CI will cover the rest).
Mid-turn steering (queued user messages, task notifications) can interleave
a non-tool message between an assistant tool_calls message and part of its
results: [assistant(tc:c1,c2), tool(c1), user, tool(c2)]. Strict upstream
validators reject that with 400 "An assistant message with 'tool_calls' must
be followed by tool messages responding to each 'tool_call_id'. (insufficient
tool messages following tool_calls message)" — reproduced live against
DeepSeek v4-flash thinking through the /v1/responses -> chat downgrade.
fixMissingToolResponses's run collection already covers per-id gaps inside
the contiguous run, but a result displaced past the run still reaches the
upstream (and now collides with the synthetic empty inserted for its id).
repairDisplacedToolResults hoists such results back to the end of the turn's
contiguous run before the pair fixes run, stopping at the next assistant
message. Registered in both translateRequest stages.
The failing checks are the same base-reds other branches see, not this diff: merged PR #18698's Lint job fails with the identical signatures ([cycles] FAIL - detected 14 strongly connected component(s) across 5992 files, Provider asset provenance failed), plus the same Docs Sync / API Route Typecheck / Quality Gate failures. No lint or cycle output references the three files this PR touches. Local verification: 8 new tests and the existing 9-case translator-toolcall-helper.test.ts suite pass under node --experimental-strip-types --test (full runner env unavailable locally; CI covers the rest).
…r callbacks
typecheck:core reported TS18047 (turnIds possibly null) at the two callbacks in repairDisplacedToolResults because the variable is reassigned later in the loop. Use a non-null alias for the open turn window; behavior is unchanged.
Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Auto incremental reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Summary
Summary by CodeRabbit
Bug Fixes
Improved handling of tool responses that appear out of order, keeping them with the related tool calls when possible.
Preserved existing behavior for responses that are already correctly paired and for conversations with missing responses.
The translator now repairs displaced tool results before applying missing-response repair in both the initial normalization pass and the final outbound-payload pass. New unit tests cover result ordering, unresolved calls, assistant-message boundaries, Claude-format results, and unchanged paired conversations.
Changes: Tool Result Repair
Layer / File(s)
Summary
Repair logic and test coverage open-sse/translator/helpers/toolCallHelper.ts, tests/unit/translator-displaced-tool-results.test.ts
The new helper moves matching tool results before intervening messages in the same assistant turn. Tests cover displaced and unresolved results, message boundaries, relative ordering, Claude-format results, and unchanged paired conversations.
Requests with tool results and user text in the same message may present that text out of order. The risk is bounded and does not block merging.
Security architecture risk: Low · up to 1baaf
The repair is bounded to conversation history and does not introduce new execution privileges. However, a message containing both a tool result and ordinary user text can move that text ahead of earlier steering instructions, changing their apparent chronology.
Retained concerns
Low · architecture · observed: A Claude user message qualifies for hoisting when any tool_result block matches the current turn, but the entire message moves. Consequently, ordinary text bundled with that result can cross earlier user steering messages. Downstream conversion separates the result from the text without restoring the original user-message chronology. This changes the interpretation context for potentially safety-relevant steering or cancellation instructions; an execution bypass has not been demonstrated.
Security review details
Security Blast Radius
inferred — The demonstrated effect concerns the supplied conversation transcript and its provider-facing interpretation. Broader tenant, credential, infrastructure, or data-store exposure was not established; propagation through caller-held array aliases remains unresolved.
Security Findings and Attack Paths
inferred — A sender able to supply a mixed Claude result carrier can make its ordinary text move ahead of intervening user instructions. This supports an instruction-chronology concern, not a verified privilege escalation or unauthorized tool-execution path.
Trust Boundaries and Controls
observed — Matching call IDs and the next-assistant stop condition constrain the repair window. These controls prevent this pass from reaching beyond the next assistant boundary, but do not distinguish ordinary user content from tool-result content within an admitted carrier.
Resilience and Maintainability Implications
inferred — For normal unique-result input, repaired carriers become contiguous and repeated repair is stable. Duplicate IDs remain outside the helper's guarantees: contiguous results do not consume the tracked IDs, and missing-result completion checks coverage rather than uniqueness. This limitation was not established as a new security regression.
Pre-merge checks: 5 · 2
Failed checks: 2 warnings
Check name
Status
Explanation
Resolution
Description check
The description covers the problem, the fix, and the testing done, and it lists the new test file. It does not follow the repository template. The Summary, Related Issues, Target Branch, Validation ch…
Add the template sections. Specifically: (1) add a Summary with the user-facing change; (2) fill in Related Issues with "Closes #" or "Related to #", or state that none applies; (3) confirm the Target Branch checklist, including that the ba…
Docstring Coverage
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 5 passed
Check name
Status
Explanation
Title check
The title follows the Conventional Commits format with type "fix" and scope "translator". It states the main change: hoisting displaced tool results into their assistant turn, which matches the added …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Tests For Production Code
Production code changed under open-sse/ in toolCallHelper.ts and translator/index.ts. The PR adds tests/unit/translator-displaced-tool-results.test.ts with coverage for the new repair behavior…
No Ai Attribution
Pass. The five reviewed commit messages contain one Co-authored-by trailer, naming diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>, which is a human repository contributor, not an AI …
Full details: Title check
Explanation
The title follows the Conventional Commits format with type "fix" and scope "translator". It states the main change: hoisting displaced tool results into their assistant turn, which matches the added repairDisplacedToolResults pass.
Full details: Tests For Production Code
Explanation
Production code changed under open-sse/ in toolCallHelper.ts and translator/index.ts. The PR adds tests/unit/translator-displaced-tool-results.test.ts with coverage for the new repair behavior and its integration with fixMissingToolResponses, including displaced results, missing results, assistant boundaries, Claude-format carriers, ordering, and no-op behavior.
Full details: No Ai Attribution
Explanation
Pass. The five reviewed commit messages contain one Co-authored-by trailer, naming diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>, which is a human repository contributor, not an AI assistant or bot. No commit contains a Generated with <AI tool> footer. The supplied PR body contains neither prohibited form.
Full details: Description check
Explanation
The description covers the problem, the fix, and the testing done, and it lists the new test file. It does not follow the repository template. The Summary, Related Issues, Target Branch, Validation checklist, Tests Added Or Updated, Coverage Notes, and Reviewer Notes sections are missing or unfilled. The Related Issues section has no issue links. The Target Branch checklist is not addressed. The Coverage Notes do not say which tests cover the changes to open-sse/. The description also does not state whether the production code change is covered by the new tests or how the lint gate ran.
Resolution
Add the template sections. Specifically: (1) add a Summary with the user-facing change; (2) fill in Related Issues with "Closes #" or "Related to #", or state that none applies; (3) confirm the Target Branch checklist, including that the base is the active release/v3.8.x branch; (4) check the Validation items, including the change type and whether npm run lint was run; (5) list the changed test file under Tests Added Or Updated; (6) explain in Coverage Notes which tests cover the open-sse/translator changes; (7) add Reviewer Notes on risky areas, such as the reordering of messages in translateRequest.
Full details: Docstring Coverage
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Extracts the forward scan and the in-place hoist out of
repairDisplacedToolResults so it stays under the complexity and
cognitive-complexity ratchets. No behavior change.
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
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
Clients that steer mid-turn (queued user messages, task notifications) can interleave a non-tool message between an assistant
tool_callsmessage and part of its results:Strict upstream validators reject the translated chat payload with a hard 400. Confirmed live against DeepSeek v4-flash (thinking) through the
/v1/responsesto chat downgrade, from a real session that ran on a Responses-native provider and then switched models:The shape beats both existing pair fixes:
fixMissingToolResponsescollects the contiguous result run after the assistant message, so a result displaced past a non-tool message is never counted and a synthetic empty is inserted for its id.Duplicate value for 'tool_call_id').stripOrphanedToolResultskeeps it because its call id exists somewhere in the conversation.Fix
New
repairDisplacedToolResultspass, registered before the pair fixes in bothtranslateRequeststages. For each assistant turn with tool calls it scans ahead (stopping at the next assistant message) and hoists any response messages that resolve the turn's remaining call ids back to the end of the turn's contiguous result run. Response carriers are matched throughrespondedToolIds, so both OpenAIrole:"tool"messages and Claude-formattool_resultuser messages are handled.After the pass, the displaced example above normalizes to:
which every upstream accepts, with the real result content preserved instead of a synthetic empty.
Testing
tests/unit/translator-displaced-tool-results.test.ts(8 cases): displaced hoist, partial coverage, dangling mid-conversation, trailing pending call, no cross-assistant hoisting, multi-result stable ordering, Claude-format carriers, fully-paired no-op.tests/unit/translator-toolcall-helper.test.tspasses unchanged (9/9) undernode --experimental-strip-types --test(full runner env unavailable locally; CI will cover the rest).