fix: drop trailing assistant message when it is empty after rstrip - #7931
Humphrey (HumphreySun98) wants to merge 1 commit into
Conversation
`_rstrip_last_assistant_message` (in both the Anthropic and OpenAI clients, used for Claude models) was documented to "Remove the last assistant message if it is empty", but it only stripped trailing whitespace from the content. A trailing assistant message that is empty (or becomes empty after stripping) was still sent, and the Anthropic API rejects a final assistant message with empty text content. The method also mutated the caller's message objects in place. Strip the trailing whitespace and, when the content becomes empty, drop the message instead of sending an empty one. A preceding non-empty assistant message (intentional prefill) is preserved, so this does not regress prefill use cases. The input messages are now copied rather than mutated. Fixes microsoft#7768 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@microsoft-github-policy-service agree |
ErenAta16 (ErenAta16)
left a comment
There was a problem hiding this comment.
Ran both new tests on the branch — 3 passed in each of the Anthropic and OpenAI test files, including the mutation guard. That last part is worth calling out on its own: the old code mutated messages[-1].content on the caller's own message objects in place, which is a separate, subtler bug from the empty-message crash — if the caller reused that message list for logging or a retry, they'd see the stripped version even though they never asked for it to be modified. model_copy(update=...) fixes both issues in one pass.
|
Friendly ping Victor Dibia (@victordibia) — this PR and a few sibling bug-fix PRs (#7930, #7933, #7947, #7948) have been open for a few weeks. This one and #7933 have been verified and approved by a community reviewer; as a first-time contributor I'd also need the CI workflows approved to run. Happy to address any feedback whenever you have bandwidth — no rush, and thanks for maintaining the project! 🙏 |
|
Following up on the ping above, now six weeks old, with a more precise ask. cc chetantoshniwal Victor Dibia (@victordibia) Eric Zhu (@ekzhu) Five small bug-fix PRs from July are ready but cannot progress without a write-access action: the
Concretely needed per PR: (1) "Approve and run workflows" so the 18 required checks run, (2) an approving review. All are single-commit, rebased on current |
|
Closing and immediately reopening to re-create the expired CI workflow runs (explanation follows). |
|
Correction to my note above, after checking the Actions API: the CI gate on these PRs was not "runs waiting to be approved" — there were no runs at all. GitHub expires fork pull-request workflow runs that sit unapproved for 30 days, and those expired records are eventually purged. The runs created when these PRs opened in July expired in August, so as of this morning all five carried zero workflow runs. That distinction matters: there was nothing for a maintainer to approve, and a review alone would not have helped either, because the 18 required status checks can never report on a commit that has no runs. I have reopened all five at their existing commits to re-create the runs: #7930, #7931, #7933, #7947, #7948. No commits were changed, nothing was force-pushed, and the existing community approvals are intact. Each now has 7 runs in So "Approve and run workflows" is genuinely available on each one now, where before it had nothing to act on. One caveat: that state expires again after 30 days, so the useful window runs to roughly mid-October. |
Why are these changes needed?
_rstrip_last_assistant_message(used for Claude models in both the Anthropicand OpenAI clients) is documented to "Remove the last assistant message if it is
empty", but it only stripped trailing whitespace from the content. A trailing
assistant message that is empty — or that becomes empty after stripping — was
still sent, and the Anthropic API rejects a final assistant message with empty
text content. The method also mutated the caller's message objects in place.
This PR strips the trailing whitespace and, when the content becomes empty,
drops the message instead of sending an empty one. A preceding non-empty
assistant message (intentional prefill) is preserved, so this does not regress
prefill use cases. The input messages are copied rather than mutated.
Note: the issue's suggested fix (drop all trailing assistant messages) would
break legitimate assistant-prefill flows; this change only drops the message
when it is actually empty, matching the method's documented intent.
Related issue number
Closes #7768
Related PRs
Checks
ruff,mypy, and the relevantpytestlocally).