feat: enforce canonical reasoning messages - #148
Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes the shared assistant-message contract across many production renderers, rejecting legacy forms and altering Kimi’s serialized reasoning representation. The broad compatibility and runtime blast radius warrants human review. Notes:
You can add or adjust custom eligibility rules. Learn more. |
hallerite
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit be20242. Configure here.
| if message.get("role") != "assistant": | ||
| continue | ||
|
|
||
| if "reasoning" in message: |
There was a problem hiding this comment.
I wonder if we should give escape hatches to bypass these checks? Forcing users to re-create their datasets can be pretty onerous.
There was a problem hiding this comment.
i think we should support reasoning and reasoning_content, myb even thinking. i have seen all of these in the wild
| "<think>" in fragment or "</think>" in fragment | ||
| for fragment in text_fragments | ||
| ): | ||
| raise ValueError( |
There was a problem hiding this comment.
Similar edge-case concerns here: I've definitely seen datasets with some stray <think> tokens in assistant content messages. It's good to know when it happens, but also maybe annoying to hard error on them.
It's also logically possible to me that the assistant might have <think> in its legitimate reasoning content, e.g. if the user is asking about LLM chat formatting. Is that wrong?
| delimiters into ``reasoning_content``. | ||
| """ | ||
| value = message.get("reasoning_content") | ||
| return value if isinstance(value, str) else "" |
There was a problem hiding this comment.
maybe warn about dropping non-string content?
garrett361
left a comment
garrett361
left a comment
There was a problem hiding this comment.
Left a few comments asking about hard errors and edge cases, but otherwise looks good! Please address before deciding to merge or not.
| if message.get("role") != "assistant": | ||
| continue | ||
|
|
||
| if "reasoning" in message: |
There was a problem hiding this comment.
i think we should support reasoning and reasoning_content, myb even thinking. i have seen all of these in the wild
| ) | ||
|
|
||
| reasoning = message.get("reasoning_content") | ||
| if reasoning is not None and not isinstance(reasoning, str): |
There was a problem hiding this comment.
also None types should be allowed and just ignored imo
3 participants
Summary
reasoning_content,content, andtool_callsreasoning,thinkingcontent parts, and inline<think>...</think>markup with normalization guidancesupports_reasoning_contenton every renderer and make DeepSeek V3, Qwen3-VL, Llama 3, and the opaque default renderer reject non-empty structured reasoning instead of silently dropping itParsedResponsefieldsRationale
Legacy datasets should be normalized before they reach a renderer. Renderers should not guess whether
<think>text inside assistant content is reasoning or literal visible content, and non-reasoning templates should never discard a populatedreasoning_contentfield without telling the caller.This intentionally declines backward compatibility with inline reasoning input, including the approach proposed in #147. Dataset adapters own legacy-to-canonical conversion; typed renderers own canonical-to-native serialization.
Tests
git diff --checkpassNote
Enforce canonical reasoning messages across all renderers
validate_canonical_messages()in base.py that rejects legacyreasoningfields,thinkingcontent parts, unsupportedreasoning_content, and inline<think>markup in assistant message history.get_structured_reasoning()helper that reads onlyreasoning_content; all renderers replace ad-hoc inline<think>parsing and legacy field fallbacks with this single call.supports_reasoning_content: boolto theRendererprotocol; non-reasoning renderers (DeepSeek V3, Qwen3-VL, Llama 3,DefaultRenderer) now raiseValueErrorwhen givenreasoning_content.ThinkingParttype andreasoningfield from theMessageschema;ContentPartnow only acceptsTextPart | ImagePart | VideoPart.reasoningfields,thinkingcontent parts, or inline<think>markup in historical assistant turns will now receive aValueErrorat render time.Macroscope summarized 6afbd78.
Note
Medium Risk
This is a breaking input contract for legacy datasets and any caller relying on inline think tags or silent dropping of
reasoning_content; behavior is intentional but will fail fast at render time across many model families.Overview
This PR tightens the assistant-message contract so render input must use
reasoning_content,content, andtool_callsonly—no guessing from wire-format text incontent.Validation and API: Adds
validate_canonical_messages(called from each renderer’srenderpath) andget_structured_reasoning, which reads onlyreasoning_content. TheRendererprotocol gainssupports_reasoning_content. Legacyreasoning,thinkingcontent parts, and inline<think>markup incontentraise with normalization guidance;ThinkingPartand thereasoningfield are removed from the public message types.Behavior: Reasoning-capable renderers project
reasoning_contentinto native wire formats (including Kimi K2, which previously ignored structured reasoning). DeepSeek V3, Qwen3-VL, Llama 3, andDefaultRenderersetsupports_reasoning_content = Falseand error on non-emptyreasoning_contentinstead of dropping it. Explicit raw passthrough modes (e.g. Lagunarender_assistant_messages_raw, Hy3raw_last_assistant) still allow inline markup on raw turns but reject structured reasoning when the mode cannot represent it.Tests/docs: Parity drops the inline-thinking-history scenario, adds Kimi K2 reference projection, and introduces
test_structured_reasoning.pyplus updates across bridge, roundtrip, and model-specific tests.Reviewed by Cursor Bugbot for commit 6afbd78. Bugbot is set up for automated code reviews on this repo. Configure here.