Skip to content

fix(fireworks): set finish_reason=tool_calls for content-embedded tool calls - #33037

Open
abhay-codes07 wants to merge 2 commits into
BerriAI:litellm_oss_daily_2026_07_10from
abhay-codes07:fix/fireworks-tool-call-finish-reason
Open

fix(fireworks): set finish_reason=tool_calls for content-embedded tool calls#33037
abhay-codes07 wants to merge 2 commits into
BerriAI:litellm_oss_daily_2026_07_10from
abhay-codes07:fix/fireworks-tool-call-finish-reason

Conversation

@abhay-codes07

Copy link
Copy Markdown

Relevant issues

Fixes #33036

Pre-Submission checklist

  • I have added meaningful tests
  • My PR passes lint / format / unit tests locally
  • My PR's scope is as isolated as possible; it only solves 1 specific problem

Screenshots / Proof of Fix

Reproducible offline (no keys/network).

Before (base 3d63eda) — tool call present but finish_reason wrong:

TOOL-CALL : tool_calls=['get_weather'] | finish_reason='stop'

After (this PR):

TOOL-CALL : tool_calls=['get_weather'] | finish_reason='tool_calls'
PLAIN-TEXT: content='Just a normal text answer.' | finish_reason='stop'   # unaffected

New regression tests (the tool-call one fails on base, passes here) + full Fireworks file green:

tests/test_litellm/llms/fireworks_ai/chat/test_fireworks_ai_chat_transformation.py  50 passed
  (incl. test_transform_response_sets_finish_reason_tool_calls_for_content_tool_call
         test_transform_response_keeps_stop_for_plain_text)

Type

🐛 Bug Fix

Changes

transform_response builds the ModelResponse directly (bypassing convert_to_model_response_object) and converts Fireworks' content-embedded tool call into message.tool_calls without updating finish_reason. Set finish_reason="tool_calls" after the conversion when a tool call is present and finish_reason is still "stop" — matching the shared response converter and providers like Ollama/xAI. Plain-text responses are unaffected.


cc @ishaan-jaff @krrish-berri-2 — isolated Fireworks tool-calling fix with regression tests. Thanks!

…l calls

Fireworks AI returns tool calls as a JSON string in message.content with finish_reason='stop' (issue BerriAI#7209). transform_response builds the ModelResponse directly via ModelResponse(**completion_response), bypassing convert_to_model_response_object, then moves the content into message.tool_calls without updating finish_reason. The response therefore had tool_calls set but finish_reason='stop', so OpenAI-style agent loops (and litellm routing) that branch on finish_reason=='tool_calls' treated the turn as finished and never executed the tool.

After converting content to tool_calls, set finish_reason to 'tool_calls' when a tool call is present and finish_reason is still 'stop' - matching the shared converter (convert_dict_to_response) and providers like Ollama/xAI. Plain-text responses are unaffected. Adds regression tests for both cases.
Copilot AI review requested due to automatic review settings July 13, 2026 04:57

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes Fireworks AI's non-streaming transform_response path, which was leaving finish_reason=\"stop\" even after a content-embedded tool call was promoted into message.tool_calls, breaking agent loops that branch on finish_reason == \"tool_calls\".

  • Adds a post-conversion check in transform_response: if finish_reason is still \"stop\" and message.tool_calls is populated, it is overwritten to \"tool_calls\", matching the contract of the shared OpenAI-style converter and providers like Ollama/xAI.
  • Introduces two offline regression tests: one that asserts the flip occurs for a content-embedded tool call, and one that confirms plain-text responses remain "stop".

Confidence Score: 5/5

The change is a small, focused one-liner inside a Fireworks-specific code path with no impact on other providers.

The fix is correctly scoped to transform_response (the non-streaming path where the bug existed), does not touch shared infrastructure, and is covered by two new offline regression tests. The only finding is a minor redundant length guard.

No files require special attention.

Important Files Changed

Filename Overview
litellm/llms/fireworks_ai/chat/transformation.py Adds finish_reason="tool_calls" correction after content-embedded tool calls are promoted to message.tool_calls; logic is correct and well-scoped to the non-streaming path.
tests/test_litellm/llms/fireworks_ai/chat/test_fireworks_ai_chat_transformation.py Adds two focused regression tests: one confirming finish_reason flips to "tool_calls" when content holds a tool call, one confirming plain-text responses remain "stop". Both are offline/mock-only.

Reviews (1): Last reviewed commit: "fix(fireworks): set finish_reason=tool_c..." | Re-trigger Greptile

Comment on lines +543 to +547
if (
typed_choice.finish_reason == "stop"
and typed_choice.message.tool_calls
and len(typed_choice.message.tool_calls) > 0
):

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.

P2 The len(typed_choice.message.tool_calls) > 0 guard is redundant — the preceding truthiness check typed_choice.message.tool_calls already returns False for both None and an empty list, so the length check adds no new protection.

Suggested change
if (
typed_choice.finish_reason == "stop"
and typed_choice.message.tool_calls
and len(typed_choice.message.tool_calls) > 0
):
if (
typed_choice.finish_reason == "stop"
and typed_choice.message.tool_calls
):

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@abhay-codes07

Copy link
Copy Markdown
Author

Heads up: the red auth-and-jwt check here is a pre-existing failure on the litellm_oss_daily_2026_07_10 base (the test_can_key_call_model_wildcard_access/test_can_team_access_model bedrockz/* cases), unrelated to this change. Root-caused and fixed separately in #33031; this PR's own changes are fully covered and green. Once #33031 lands in the base, this check goes green on re-run.

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

The truthiness check on message.tool_calls already excludes None and empty lists, so the explicit len(...) > 0 is redundant (per Greptile review on BerriAI#33037).
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.

2 participants