Skip to content

Conversation

@SongChiYoung
Copy link
Contributor

Why are these changes needed?

This PR fixes a 400 - invalid_request_error that occurs when using Anthropic models and the final message is from the assistant and ends with trailing whitespace.

Example error:

Error code: 400 - {'error': {'code': 'invalid_request_error', 'message': 'messages: final assistant content cannot end with trailing whitespace', ...}}

To unblock ongoing internal usage, this patch introduces an ad-hoc fix that strips trailing whitespace if the model is Anthropic and the last message is from the assistant.

Related issue number

Ad-hoc fix for issue discussed here:
#6167

Follow-up structural proposal here:
#6167
#6167 (comment)

Checks

Copy link
Contributor

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

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

Why not use the message transformation?

@SongChiYoung
Copy link
Contributor Author

SongChiYoung commented Apr 1, 2025

@ekzhu

Thanks for the question! (I wrote it quickly at first, but just updated the comment now.)

Why not use the message transformation?

The reason I didn't use the transformer is because this issue specifically applies to the last AssistantMessage, and the current message transformation logic works on a per-message basis — it doesn't have visibility into the full message sequence.

If we apply .rstrip() blindly to all AssistantMessages in the transformer stage, it could have unintended side effects, such as:

["show under message\n", "blah, blah"] → ["show under message", "blah, blah"]

That’s why I applied the fix after the message list is fully constructed — to avoid over-sanitizing content that’s meant to preserve formatting.

Let me know if you’d prefer a different way to organize this logic!

@SongChiYoung
Copy link
Contributor Author

(Aside: Why did we originally design the transformer to work per-message?)

That was intentional — based on the Single Responsibility Principle.

Message transformers are meant to map the structure of each LLMMessage into SDK-specific args (e.g., role, name, content) in isolation. They're responsible for how each message "looks" structurally, not how the message sequence should be interpreted as a whole.

In contrast, message stream-level rules — like "strip trailing space from the last AssistantMessage" — depend on list-level context and vendor quirks. So they should live in a separate layer like MessageCleaner, which operates on the full message sequence.

This keeps responsibilities clear:

  • Transformer → adapts message structure
  • Cleaner → sanitizes message list

@ekzhu
Copy link
Contributor

ekzhu commented Apr 1, 2025

@ekzhu

Thanks for the question! (I wrote it quickly at first, but just updated the comment now.)

Why not use the message transformation?

The reason I didn't use the transformer is because this issue specifically applies to the last AssistantMessage, and the current message transformation logic works on a per-message basis — it doesn't have visibility into the full message sequence.

If we apply .rstrip() blindly to all AssistantMessages in the transformer stage, it could have unintended side effects, such as:

["show under message\n", "blah, blah"] → ["show under message", "blah, blah"]

That’s why I applied the fix after the message list is fully constructed — to avoid over-sanitizing content that’s meant to preserve formatting.

Let me know if you’d prefer a different way to organize this logic!

Understood, let's starts with the design in this PR and wait for usages and feedbacks.

@SongChiYoung
Copy link
Contributor Author

Sounds good - thanks!
Let’s see how it goes.

@ekzhu ekzhu enabled auto-merge (squash) April 2, 2025 00:54
@ekzhu ekzhu merged commit 9de16d5 into microsoft:main Apr 2, 2025
56 checks passed
@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.31%. Comparing base (aec04e7) to head (3fd2fab).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../autogen_ext/models/anthropic/_anthropic_client.py 71.42% 2 Missing ⚠️
...xt/src/autogen_ext/models/openai/_openai_client.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6168      +/-   ##
==========================================
+ Coverage   77.26%   77.31%   +0.04%     
==========================================
  Files         197      197              
  Lines       13783    13797      +14     
==========================================
+ Hits        10650    10667      +17     
+ Misses       3133     3130       -3     
Flag Coverage Δ
unittests 77.31% <78.57%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ekzhu added a commit that referenced this pull request Apr 4, 2025
Please refer to #6123  for full context.

That issue outlines several design and behavioral problems with
`SocietyOfMindAgent`.
This DRAFT PR focuses on resolving the most critical and broken
behaviors first.

Here is the error list
🔍 SocietyOfMindAgent: Design Issues and Historical Comparison (v0.2 vs
v0.4+)

### ✅ P1–P4 Regression Issue Table (Updated with Fixes in PR #6142)

| ID | Description | Current v0.4+ Issue | Resolution in PR #6142 | Was
it a problem in v0.2? | Notes |

|-----|-------------|----------------------|--------------------------|----------------------------|-------|
| **P1** | `inner_messages` leaks into outer team termination evaluation
| `Response.inner_messages` is appended to the outer team's
`_message_thread`, affecting termination conditions. Violates
encapsulation. | ✅ `inner_messages` is excluded from `_message_thread`,
avoiding contamination of outer termination logic. | ❌ No | Structural
boundary is now enforced |
| **P2** | Inner team does not execute when outer message history is
empty | In chained executions, if no new outer message exists, no task
is created and the inner team is skipped entirely | ✅ Detects absence of
new outer message and reuses the previous task, passing it via a handoff
message. This ensures the inner team always receives a valid task to
execute | ❌ No | The issue was silent task omission, not summary
failure. Summary succeeds as a downstream effect |
| **P3** | Summary LLM prompt is built from external input only | Prompt
is constructed using external message history, ignoring internal
reasoning | ✅ Prompt construction now uses
`final_response.inner_messages`, restoring internal reasoning as the
source of summarization | ❌ No | Matches v0.2 internal monologue
behavior |
| **P4** | External input is included in summary prompt (possibly
incorrectly) | Outer messages are used in the final LLM summarization
prompt | ✅ Resolved via the same fix as P3; outer messages are no longer
used for summary | ❌ No | Redundant with P3, now fully addressed |


<!-- Thank you for your contribution! Please review
https://microsoft.github.io/autogen/docs/Contribute before opening a
pull request. -->

<!-- Please add a reviewer to the assignee section when you create a PR.
If you don't have the access to it, we will shortly find a reviewer and
assign them to your PR. -->

## Why are these changes needed?

<!-- Please give a short summary of the change and the problem this
solves. -->

## Related issue number
resolve #6123 
Blocked #6168 (Sometimes SoMA send last whitespace message)
related #6187
<!-- For example: "Closes #1234" -->

## Checks

- [ ] I've included any doc changes needed for
<https://microsoft.github.io/autogen/>. See
<https://github.com/microsoft/autogen/blob/main/CONTRIBUTING.md> to
build and test documentation locally.
- [ ] I've added tests (if relevant) corresponding to the changes
introduced in this PR.
- [ ] I've made sure all auto checks have passed.

---------

Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
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