Repository navigation
fix(models): handle Mistral list content as reasoning and text blocks (py) - #5487
Conversation
|
@mittalpk Hi, thanks for the contribution. I left a few comments. |
ea7ec39 to
3cbfc47
Compare
|
Thanks, all three done: |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Approve — correct fix, no blocking findings.
I reproduced #5032 on the merge-base (TypeError: can only concatenate str (not "list") to str at event_loop/streaming.py:244) and confirmed it's gone on head 3cbfc474c, including the reasoning→tool_use turn that the removed smoke xfail covered. Content blocks stay balanced across every streaming shape I threw at it, and the replayed requests validate against the real mistralai models.
Both halves match Mistral's docs. Streaming: thinking list → transition list → plain string (reasoning docs). Request side: the docs make the replay mandatory, not optional — "always replay the full assistant message (including ThinkChunk) back into the message history" — so _format_request_messages sending {"type": "thinking", ...} is the right call, and it's the same transform Mistral's own Vibe client, @ai-sdk/mistral and the litellm fix (BerriAI/litellm#45546) use.
One non-blocking test-gap suggestion inline. I could not run the live integ test (no MISTRAL_API_KEY in my sandbox), so the TOOL_USE_MARKS xfail removal is unverified against the real API — worth one live run before merge.
✅ What I verified (head 3cbfc474c vs merge-base 15ca21f15)
Gates: pytest tests/strands/models/test_mistral.py tests/strands/event_loop → 264 passed; ruff check + ruff format --check clean; mypy src/strands/models/mistral.py clean. (Other provider test modules don't collect in my sandbox — missing optional deps, unrelated.)
The tests prove behaviour, not the implementation: all 5 new tests fail on the merge-base with the PR's source reverted. The base failure output is the bug itself — {'contentBlockDelta': {'delta': {'text': [ThinkChunk(...), ReferenceChunk(...), TextChunk(...)]}}}.
Adversarial repro (full agent loop, fake Mistral client built from real mistralai 3.6.0 chunk classes, asserting contentBlockStart/Stop balance and validating each outgoing request against ChatCompletionRequest):
| Scenario | Result |
|---|---|
Documented shape: thinking list → transition list ([closing ThinkChunk, TextChunk]) → plain-string answer |
balanced; records reasoningContent then text |
Reasoning + tool_calls, then reasoning + final answer (2 turns) |
balanced; turn-2 request replays [{"type":"thinking",...}] + tool_calls and validates against the SDK |
Empty-string deltas and empty TextChunk interleaved |
no stray block open/close (matches pre-PR falsy-skip) |
text → thinking → text interleaving in one message |
3 balanced blocks, order preserved into agent.messages |
Stream ends with no finish_reason |
block left open and content dropped — pre-existing, identical on main (content_started was never reset either); not caused by this PR |
Consistency with other providers: reasoning_content as a data_type matches openai.py:587 and llamacpp.py:531, and _content_events mirrors openai.py:825 _stream_switch_content (same "caller closes the open block" contract). No public surface changes — both helpers are private; check-api-review-label is green and no API review is needed.
Evidence (repro script, its output on head, the base reproduction, gate log) is attached to this PR in the portal.
Questions (none blocking)
- After this change, a non-reasoning Mistral model (
ministral-*,codestral-*) will receive a thinking chunk if the history has one — e.g.update_config(model_id=...)mid-conversation, or a restored session. I looked for evidence that any Mistral model rejects athinkingchunk in an assistant message and found none, so I'm not asking for a change; just confirming you considered that path. - Reading order for a second reviewer:
mistral.py:435-484(the two helpers) → call sites500-503and584-595→ request side272-277.
Appendix — non-blocking (4)
- PR description is stale. It says the strict xfail removed was
TestMistralLarge4::test_tool_use_turnand that "the test now reads the first text block, since a reasoning block can come first" — there is noTestMistralLarge4in the repo and no test was changed that way. What the diff actually removes is themistral-large-4-0entry fromTOOL_USE_MARKS(tests_integ/models/test_model_smoke.py:37), and that test already usesblock.get("toolUse"), so a leading reasoning block is safe. Worth fixing the body before merge. (The existingtests_integ/models/test_model_mistral.py:80,88,100do indexcontent[0]["text"], but they runmistral-medium-latestwith noreasoning_effort, so they still get plain-string content.) signature/closedare dropped on replay (mistral.py:272-277), and the provider never emits areasoningContent.signaturedelta, so they're unrecoverable on a later turn. I went looking for a problem here and didn't find one:signatureisOptionalNullableonThinkChunk, the docs' multi-turn example never mentions it, and the other clients doing this transform also rebuild from text alone. Noting it only so the choice is on the record —streaming.py:265does have a signature channel if Mistral ever starts requiring it.TOOL_USE_MARKS: dict[str, pytest.MarkDecorator] = {}plus its lookup attest_model_smoke.py:56is dead until the next xfail. Fine as a hook; just flagging it.- A cross-provider
reasoningContentblock carrying onlyredactedContent(Bedrock shape) is silently skipped by the.get("reasoningText", {}).get("text")guard. Correct behaviour, and Mistral never produces that shape — mentioning it because the skip is implicit rather than counted inskipped.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Thanks for the bugfix :) |
Human Overview
Problem: Mistral Large 4 returns responses as ThinkChunk and TextChunk lists, but the SDK expected a string, causing a TypeError.
Fix: The code now handles reasoning and text chunks separately while preserving compatibility with older models in streaming and non-streaming modes.
Testing: Three new unit tests pass, and the integration test’s expected-failure marker was removed; live testing was skipped due to a missing Mistral API key.
Description
Reasoning models such as Mistral Large 4 return
contentas a list ofThinkChunk/TextChunkobjects instead of a string. MistralModel passed that list through as text, which raised aTypeErrorin the agent loop.Content is now split into parts:
ThinkChunktext becomesreasoningContentblocks,TextChunkbecomes text, and chunks without text (such as references) are skipped. Both the streaming and non-streaming paths use it, and plain string content behaves as before.Related Issues
Resolves #5032
Documentation PR
Not needed.
Type of Change
Bug fix
Testing
Added three unit tests with mocked responses: the reasoning delta, streaming list content split across chunks, and non-streaming list content including a reference chunk. They fail without the fix, and the full Mistral unit suite passes.
Removed the strict xfail on
TestMistralLarge4::test_tool_use_turn. The test now reads the first text block, since a reasoning block can come first.I could not run the integration test without a Mistral API key.
hatch run preparedid not resolve its environment locally, so I ran ruff, mypy and pytest directly.hatch run prepare