feat: Add reasoning content separation for LLM providers#136
Conversation
Sync with latest changes from m-mizutani/gollem upstream: - chore: ignore .pnpm-store directory - fix(trace): fall back to RootSpan when context has no active span - Various dependency updates and fixes Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Local working documents (specs, plans) should not be committed to the repository.
Implement OpenAI reasoning content support following TDD approach: - Add handling for ReasoningContent field in convertOpenAIMessage() to create gollem.ThinkingContent - Add handling for MessageContentTypeThinking in convertMessageToOpenAI() to set the ReasoningContent field - Add comprehensive test for round-trip conversion The implementation mirrors the existing Claude thinking content support, using the same ThinkingContent type for cross-provider compatibility. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extract reasoning content from OpenAI API responses and populate the Thinkings field in both ContentResponse and Response structs. Changes: - Add Thinkings field to gollem.Response struct - Extract ReasoningContent from message responses in non-streaming mode - Extract ReasoningContent from delta chunks in streaming mode - Include reasoning content in assistant messages for history tracking - Add tests to verify reasoning content extraction This enables proper handling of thinking/reasoning content from GPT-5 models that support the reasoning_effort parameter. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add Thinkings field to response initialization - Extract thought parts to Thinkings array instead of skipping - Convert thought parts to MessageContentTypeThinking in convert_message.go This enables Gemini thinking models to properly separate internal reasoning from final output, matching the behavior of Claude and OpenAI reasoning models. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Update TestThoughtPartsExcludedFromResponse to expect MessageContentTypeThinking - Add MessageContentTypeThinking case to convertContentToGemini - All tests now pass with thinking content separation Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rename all occurrences of "Thinkings" to "Thoughts" for better naming: - "Thought" describes the content of reasoning (like "Text" describes content) - More natural in English than "Thinkings" - Consistent with Claude API terminology Affected files: - llm.go, execute_response.go, middleware.go (core types) - llm/openai/client.go (OpenAI implementation) - llm/gemini/client.go (Gemini implementation) - All tests updated accordingly Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Complete renaming from "Thinking" to "Reasoning" terminology: - Core types: MessageContentTypeThinking → MessageContentTypeReasoning - Structs: ThinkingContent → ReasoningContent - Functions: NewThinkingContent() → NewReasoningContent() - Functions: GetThinkingContent() → GetReasoningContent() - Trace package: NewThinkingContent() → NewReasoningContent() - Trace package: NewRedactedThinkingContent() → NewRedactedReasoningContent() - All tests updated accordingly Rationale: "Reasoning" better describes the content/thought process rather than the act of thinking. All tests pass successfully. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a standardized way to handle LLM "thinking" or "reasoning" content across Claude, Gemini, and OpenAI providers by adding a new MessageContentTypeReasoning type and updating core response structures. The review feedback identifies critical issues in the Claude implementation where thinking signatures are lost and redacted blocks are mishandled, which would break multi-turn conversations. Additionally, a bug was found in the OpenAI conversion logic where reasoning content is overwritten rather than accumulated, and a local VS Code workspace file was mistakenly included in the repository.
| if block.OfThinking != nil { | ||
| return gollem.NewReasoningContent(block.OfThinking.Thinking) | ||
| } | ||
|
|
||
| // Handle redacted thinking blocks | ||
| if block.OfRedactedThinking != nil { | ||
| return gollem.NewReasoningContent(block.OfRedactedThinking.Data) | ||
| } |
There was a problem hiding this comment.
There are two issues with how Claude thinking blocks are converted to gollem.Message:
- Signature Loss: For normal thinking blocks (line 64), the
Signatureis lost. Anthropic requires this signature to be included when sending the thinking block back in subsequent messages for multi-turn conversations. - Redacted Block Bug: For redacted thinking blocks (line 69), the signature (
Data) is being stored in theTextfield ofReasoningContent. This will cause the encrypted signature to be displayed as literal thought text to the user, and it will be incorrectly sent back as a normal thinking block inconvertContentToClaude.
Consider using the Meta field to store the signature and the redacted status, similar to how it's handled in the Gemini implementation.
| case gollem.MessageContentTypeReasoning: | ||
| thinkingContent, err := content.GetReasoningContent() | ||
| if err != nil { | ||
| return anthropic.ContentBlockParamUnion{}, err | ||
| } | ||
| // Skip empty thinking content | ||
| if thinkingContent.Text == "" { | ||
| return anthropic.ContentBlockParamUnion{}, convert.ErrUnsupportedContentType | ||
| } | ||
| // Convert thinking to Claude thinking block | ||
| // NewThinkingBlock(signature, thinking) - note parameter order! | ||
| return anthropic.NewThinkingBlock("", thinkingContent.Text), nil |
There was a problem hiding this comment.
When converting gollem.MessageContentTypeReasoning back to Claude format, the code always creates a normal ThinkingBlock with an empty signature. This breaks multi-turn conversations (which require the signature from the previous turn) and incorrectly handles redacted blocks (which should be sent back as RedactedThinkingBlock).
| return nil, goerr.Wrap(err, "failed to get thinking content") | ||
| } | ||
| // Store reasoning content separately (will be set on the main message) | ||
| reasoningContent = thinkingContent.Text |
There was a problem hiding this comment.
The reasoningContent variable is overwritten in each iteration of the loop. If a message contains multiple reasoning content blocks, only the last one will be preserved when converting back to the OpenAI format. It should be accumulated instead.
| reasoningContent = thinkingContent.Text | |
| reasoningContent += thinkingContent.Text |
| { | ||
| "folders": [ | ||
| { | ||
| "path": "." | ||
| }, | ||
| { | ||
| "path": "/home/denkhaus/dev/dendron" | ||
| } | ||
| ], | ||
| "settings": {} | ||
| } |
**Claude Implementation:** - Add claudePartMeta struct to preserve thinking signatures and redacted status - Store signature in Meta field for normal thinking blocks (multi-turn support) - Store signature and redacted flag in Meta for redacted thinking blocks - Fix round-trip conversion to restore signatures and handle redacted blocks correctly **OpenAI Implementation:** - Fix reasoning content accumulation (was overwriting, now using +=) **Tests:** - Update redacted thinking block test to expect empty text with meta field - Remove VS Code workspace file that shouldn't be in repository These changes address critical issues found in PR review that would break multi-turn conversations and mishandle redacted thinking blocks. All tests pass successfully. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@denkhaus As of May 2026, the upstream providers use the following terminology:
Two of three vendors use thinking-family wording, and our existing public API already uses Thinking. Given there's no clear industry standard to align with, I'm hesitant about the breaking rename in the trace package without a stronger upside. How do you feel about keeping Thinking as the vocabulary across the codebase (including the new additions)? Curious to hear your take. |
|
Thank you for the thoughtful feedback and analysis! You make excellent points:
I am happy to revert the terminology change from "reasoning" back to "thinking". This will:
I will prepare a follow-up commit to change the naming back to "thinking" throughout the codebase. The core implementation logic (signatures, redacted blocks, accumulation) will remain the same - only the naming will change. Would you like me to proceed with this terminology update? |
Per upstream maintainer request, revert terminology from "reasoning" to "thinking" to align with: - Provider terminology: 2/3 vendors (Claude, Gemini) use "thinking" - Existing trace package API: already uses "Thinking" - Industry consistency: no clear standard, prefer staying with existing Changes: - MessageContentTypeReasoning → MessageContentTypeThinking (value: "thinking") - ReasoningContent → ThinkingContent - NewReasoningContent() → NewThinkingContent() - GetReasoningContent() → GetThinkingContent() - Trace package: NewReasoningContent() → NewThinkingContent() - Trace package: NewRedactedReasoningContent() → NewRedactedThinkingContent() **Kept unchanged:** - Response.Thoughts (already correct - plural like "Texts") **OpenAI-specific:** - Kept OpenAI library field names as ReasoningContent (library uses this) - Only our wrapper code uses Thinking terminology All tests pass successfully. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@denkhaus |
|
I hope the commit fits nicely in your vision of gollem. So with the last commit, I changed the wording back to "Thinking". However, in the response, I still use |
|
@denkhaus I'll go ahead and merge this. |
Summary
This PR adds support for separating reasoning/thinking content from regular output across all LLM providers (Claude, OpenAI, Gemini). It enables applications to distinguish between internal model reasoning and final responses.
Motivation
Why this matters:
As LLMs with reasoning capabilities become more prevalent (Claude's extended thinking, OpenAI's o1/o3 models, Gemini's thinking models), applications need to:
Without this separation:
Changes
Core Types (
message.go) - ✨ NEWMessageContentTypeReasoningconstant ("reasoning")ReasoningContentstructNewReasoningContent()helper functionGetReasoningContent()helper functionResponse Struct (
llm.go) - ✨ NEWThoughts []stringfield toResponsestruct for storing reasoning contentProvider Implementations
Claude (
llm/claude/):OfThinkingblocks toMessageContentTypeReasoningResponse.ThoughtsarrayOpenAI (
llm/openai/):ReasoningContentfield toMessageContentTypeReasoningResponse.ThoughtsarrayGemini (
llm/gemini/): ✨ NEWThoughtparts toMessageContentTypeReasoningResponse.ThoughtsarrayMessageContentTypeReasoningcase toconvertContentToGemini()ExecuteResponse (
execute_response.go) - ✨ NEWThoughts []stringfield for consistencyMiddleware (
middleware.go) - ✨ NEWThoughts []stringfield to handler contextTrace Package -⚠️ BREAKING CHANGE
NewThinkingContent()→NewReasoningContent()NewRedactedThinkingContent()→NewRedactedReasoningContent()"thinking"to"reasoning"Terminology
Used "Reasoning" throughout (not "Thinking") as it better describes the content of thought (like "Text" describes content), making it more natural in English.
Breaking Changes
The trace package functions have been renamed for consistency:
trace.NewThinkingContent()→trace.NewReasoningContent()trace.NewRedactedThinkingContent()→trace.NewRedactedReasoningContent()Migration guide:
Note: This is a minimal breaking change affecting only the trace package. The core API additions are purely additive.
Testing
All tests pass successfully:
message_test.go)llm/claude/)llm/openai/)llm/gemini/)trace/)Test Coverage
Core Tests:
TestMessageContentTypeReasoning- Type constant validationTestReasoningContent- Struct validationTestNewReasoningContent- Content creationTestGetReasoningContent- Content extractionProvider Tests:
Benefits
✅ Consistent API - All three providers now handle reasoning content uniformly
✅ Better UX - Applications can display reasoning separately from final output
✅ Analytics ready - Reasoning can be tracked and analyzed separately
✅ Future-proof - Ready for new reasoning models from any provider
✅ Well-tested - Comprehensive test coverage across all providers
✅ Minimal breaking changes - Only trace package affected
Example Usage
Checklist