Skip to content

fix(translator): strip orphaned tool results across request formats - #5805

Merged
diegosouzapw merged 3 commits into
release/v3.8.43from
fix/port-pr-2237-orphaned-tool-results
Jul 2, 2026
Merged

diegosouzapw merged 3 commits into
release/v3.8.43from
fix/port-pr-2237-orphaned-tool-results

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Summary

Client-side history truncation/compaction can leave a stale tool result in a request after the matching tool call was dropped from history. Strict upstream APIs reject the whole request before model execution with a 400 (e.g. "Messages with role 'tool' must be a response to a preceding message with 'tool_calls'").

The translator already enforced two thirds of the tool-call/result pairing invariant:

  • ensureToolCallIds — make call ids well-formed
  • fixMissingToolResponses — repair the inverse case (a call exists but its result is missing)

This adds the missing third leg: every tool result must reference a tool call still present in the same request.

Note: the Responses API → OpenAI direction was checked first and is already covered by an existing post-filter in openai-responses.ts (hardened previously for empty/missing call ids) — no change was needed there. Only the two genuinely missing sites were ported.

Changes

  • open-sse/translator/helpers/toolCallHelper.ts: new stripOrphanedToolResults(body), mirroring fixMissingToolResponses on the result side. Collects known call ids from assistant.tool_calls[] (OpenAI) and tool_use content blocks (Claude), then drops:
    • orphaned OpenAI role:"tool" messages with a stale tool_call_id
    • orphaned Claude-shaped tool_result content blocks with a stale tool_use_id
    • a user message entirely, if stripping empties its content array
    • Returns the same object reference when nothing needs to change (no-op fast path).
  • open-sse/translator/index.ts: wire stripOrphanedToolResults immediately after fixMissingToolResponses at both call sites (source-format pass and final target-format pass), so every route that pivots through the canonical intermediate format is covered.

Attribution

Thanks to @warelik for the original implementation.

Testing

  • New: tests/unit/translator-toolcall-helper.test.ts — matched OpenAI tool_calls/role:tool preserved; orphan role:tool stripped; zero-call truncation strips all stale results; matched Claude-shaped tool_use/tool_result preserved; orphan Claude-shaped tool_result stripped from mixed content while sibling text is preserved; a user message left with only an orphan tool_result is dropped entirely; no-op returns the same body reference; mixed matched+orphan keeps only the matched result.
  • New: tests/unit/translator-request-openai-responses.test.ts — regression coverage pinning down the already-correct Responses→OpenAI orphan-stripping behavior (matched preserved, orphan stripped, zero-function-call truncation, mixed matched+orphan).
  • Ran the full existing translator/tool-call/reasoning-replay suite (742 tests across tests/unit/translator-*.test.ts, translator-toolcall-helper, reasoning-cache*, reasoning-replay-big-pickle, xai-translators) — all pass, no regressions.
  • npx eslint on all touched/added files — clean.
  • npm run typecheck:core — clean project-wide.

Client-side history truncation/compaction can drop the assistant turn
that issued a tool call while leaving its stale tool result behind.
Strict upstream APIs reject the whole request before model execution
with a 400 ("Messages with role 'tool' must be a response to a
preceding message with 'tool_calls'").

9router already had the two other legs of the tool-call/result pairing
invariant: ensureToolCallIds (well-formed ids) and
fixMissingToolResponses (every call has a result). This adds the
missing reverse invariant: every result has a call.

- open-sse/translator/helpers/toolCallHelper.ts: new
  stripOrphanedToolResults(body), handling both OpenAI role:"tool"
  messages and Claude-shaped tool_result content blocks; drops a user
  message entirely if stripping empties its content array; no-op fast
  path returns the same body reference.
- open-sse/translator/index.ts: wire stripOrphanedToolResults
  immediately after fixMissingToolResponses at both call sites, so
  every route that pivots through the canonical intermediate format is
  covered.

The Responses API -> OpenAI direction was checked and is already
covered (see the `allToolCallIds` post-filter in
openai-responses.ts, hardened under #2893) — no change needed there;
added regression tests pinning that existing behavior down explicitly.

Co-authored-by: warelik <54947489+warelik@users.noreply.github.com>
Inspired-by: decolua/9router#2237
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the stripOrphanedToolResults helper function to remove tool results that lack a matching tool call in the conversation history, along with comprehensive unit tests. The reviewer pointed out a critical issue where tool messages with missing or empty tool_call_ids would bypass the check and be preserved in the payload, and provided a code suggestion to ensure these are correctly stripped.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +196 to +203
if (msg.role === "tool" && msg.tool_call_id) {
if (knownCallIds.has(msg.tool_call_id)) {
filteredMessages.push(msg);
} else {
changed = true;
}
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current check msg.role === "tool" && msg.tool_call_id will skip any tool message that has a missing, null, or empty tool_call_id. This means such invalid/orphaned tool messages will fall through and be preserved in the final payload, which strict upstream APIs will reject with a 400 error.

To align with the hardening previously done in openai-responses.ts (which strips any tool result with a missing/empty ID), we should check msg.role === "tool" first, and then strip it if it lacks a valid matching ID in knownCallIds.

Suggested change
if (msg.role === "tool" && msg.tool_call_id) {
if (knownCallIds.has(msg.tool_call_id)) {
filteredMessages.push(msg);
} else {
changed = true;
}
continue;
}
if (msg.role === "tool") {
if (msg.tool_call_id && knownCallIds.has(msg.tool_call_id)) {
filteredMessages.push(msg);
} else {
changed = true;
}
continue;
}

@diegosouzapw
diegosouzapw merged commit ace6479 into release/v3.8.43 Jul 2, 2026
2 of 3 checks passed
@diegosouzapw diegosouzapw mentioned this pull request Jul 2, 2026
@diegosouzapw
diegosouzapw deleted the fix/port-pr-2237-orphaned-tool-results branch July 3, 2026 04:58
tkgo11 pushed a commit to tkgo11/OmniRoute that referenced this pull request Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant