Skip to content

fix: ensure assistant messages with tool_calls include content field #7076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
katzdave merged 4 commits into from
Feb 11, 2026
Merged

fix: ensure assistant messages with tool_calls include content field #7076

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd9d2dd
fix: include content field in assistant messages with tool_calls
Feb 9, 2026
5270889
Address review feedback: simplify comment and remove verbose tests
opspawn Feb 10, 2026
fba64c1
Add tests for content: null behavior on assistant tool-call messages
opspawn Feb 10, 2026
feacc37
Remove tests per reviewer request
opspawn Feb 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crates/goose/src/providers/formats/openai.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,15 @@ pub fn format_messages(messages: &[Message], image_format: &ImageFormat) -> Vec<
converted["content"] = json!(text_array.join("\n"));
}

// Some strict OpenAI-compatible providers require "content" to be present
Copy link
Copy Markdown
Contributor

@katzdave katzdave Feb 9, 2026

Choose a reason for hiding this comment

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

Let's just leave this line of this comment describing the reason for the code.

// (even as null) when tool_calls are provided. See #6717.
if message.role == Role::Assistant
&& converted.get("tool_calls").is_some()
&& converted.get("content").is_none()
{
converted["content"] = json!(null);
}
Comment on lines +250 to +257
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The PR description claims two new tests were added (test_format_messages_tool_calls_without_text_includes_content_null / ...with_text_keeps_content_string), but those tests don’t appear anywhere in the repo; either add them or update the description to match what’s actually included.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@fl-sean03 fl-sean03 Feb 10, 2026

Choose a reason for hiding this comment

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

Fixed — PR description updated to match the actual test names. The original tests were deleted per @katzdave's request, and two new focused tests have been added: test_format_messages_tool_calls_only_sets_content_null and test_format_messages_tool_calls_with_text_keeps_content_string.

Comment on lines +252 to +257
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

This new behavior (forcing content: null when an assistant message only contains tool calls) isn’t covered by existing tests; please add/extend a unit test (e.g., update test_format_messages_complex or add a dedicated case) to assert spec[..]["content"].is_null() for assistant tool-call-only messages and that mixed text+tool calls keeps content as a string.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@fl-sean03 fl-sean03 Feb 10, 2026

Choose a reason for hiding this comment

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

Added two tests in the latest commit (cef2a90): test_format_messages_tool_calls_only_sets_content_null asserts content is present and null for assistant tool-call-only messages. test_format_messages_tool_calls_with_text_keeps_content_string asserts mixed text+tool calls keeps content as a string.


if converted.get("content").is_some() || converted.get("tool_calls").is_some() {
output.insert(0, converted);
}
Expand Down