Skip to content

fix: preserve reasoning_content for thinking models in multi-turn conversations #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
siciyuan404 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: preserve reasoning_content for thinking models in multi-turn conversations #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39998a6
fix: preserve reasoning_content for thinking models in multi-turn con…
mxrain Feb 14, 2026
53ddf35
fix: preserve ReasoningContent in toolloop for thinking models
mxrain Feb 15, 2026
ad2cce7
Merge remote-tracking branch 'origin/main' into fix/thinking-model-re…
mxrain Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/agent/loop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,9 @@ func (al *AgentLoop) runLLMIteration(ctx context.Context, messages []providers.M

// Build assistant message with tool calls
assistantMsg := providers.Message{
Role: "assistant",
Content: response.Content,
Role: "assistant",
Content: response.Content,
ReasoningContent: response.ReasoningContent, // Preserve for thinking models (e.g., GLM-Z1)
Copy link
Collaborator

@Leeaandrob Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

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

[Positive] @siciyuan404 — Good catch propagating ReasoningContent from the response into the assistant message. This is the critical path that was causing the API errors. The fact that you also caught the same pattern in toolloop.go shows thoroughness.

}
for _, tc := range response.ToolCalls {
argumentsJSON, _ := json.Marshal(tc.Arguments)
Expand Down
12 changes: 7 additions & 5 deletions pkg/providers/http_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ func (p *HTTPProvider) parseResponse(body []byte) (*LLMResponse, error) {
var apiResponse struct {
Choices []struct {
Message struct {
Content string `json:"content"`
Content string `json:"content"`
ReasoningContent string `json:"reasoning_content"` // For thinking models (e.g., GLM-Z1)
Copy link
Collaborator

@Leeaandrob Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

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

[H2 — HIGH: No Test Coverage] @siciyuan404 — This is the core parsing logic for the new feature, but there's no unit test that validates reasoning_content is correctly parsed from an API response.

The existing TestHTTPProvider_parseResponse (or similar) should have a new test case like:

func TestParseResponse_ReasoningContent(t *testing.T) {
    body := `{"choices":[{"message":{"content":"answer","reasoning_content":"thinking process"},"finish_reason":"stop"}]}`
    p := &HTTPProvider{}
    resp, err := p.parseResponse([]byte(body))
    if err != nil {
        t.Fatalf("parseResponse() error: %v", err)
    }
    if resp.ReasoningContent != "thinking process" {
        t.Errorf("ReasoningContent = %q, want %q", resp.ReasoningContent, "thinking process")
    }
}

Also need a test that verifies ReasoningContent is empty/omitted for non-thinking models.

Without tests, this fix could regress silently in future refactors (especially with PR #213 restructuring these files).

ToolCalls []struct {
ID string `json:"id"`
Type string `json:"type"`
Expand Down Expand Up @@ -186,10 +187,11 @@ func (p *HTTPProvider) parseResponse(body []byte) (*LLMResponse, error) {
}

return &LLMResponse{
Content: choice.Message.Content,
ToolCalls: toolCalls,
FinishReason: choice.FinishReason,
Usage: apiResponse.Usage,
Content: choice.Message.Content,
ReasoningContent: choice.Message.ReasoningContent,
Copy link
Collaborator

@Leeaandrob Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

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

[H3 — HIGH: Claude Provider Gap] @siciyuan404 — You correctly added ReasoningContent propagation for the HTTP (OpenAI-compatible) provider. However, the ClaudeProvider in claude_provider.go has a related gap:

parseClaudeResponse() (line 155-172) only handles block.Type == "text" and block.Type == "tool_use". Claude models with extended thinking produce block.Type == "thinking" content blocks, which are silently dropped.

While this is a pre-existing issue and Claude uses a different mechanism than reasoning_content, this PR is the natural place to add parity. The fix would be:

case "thinking":
    tb := block.AsThinking()
    // Store in ReasoningContent for consistency
    reasoningContent += tb.Thinking

This would give PicoClaw consistent thinking-model support across both protocol families (OpenAI-compat and Anthropic).

If you'd prefer to keep this PR focused, at minimum please open a follow-up issue tracking this gap.

ToolCalls: toolCalls,
FinishReason: choice.FinishReason,
Usage: apiResponse.Usage,
}, nil
}

Expand Down
18 changes: 10 additions & 8 deletions pkg/providers/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ type FunctionCall struct {
}

type LLMResponse struct {
Content string `json:"content"`
ToolCalls []ToolCall `json:"tool_calls,omitempty"`
FinishReason string `json:"finish_reason"`
Usage *UsageInfo `json:"usage,omitempty"`
Content string `json:"content"`
ReasoningContent string `json:"reasoning_content,omitempty"` // For thinking models (e.g., GLM-Z1)
Copy link
Collaborator

@Leeaandrob Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

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

[H1 — HIGH: Merge Conflict with PR #213] @siciyuan404 — This file (types.go) is also modified by PR #213 ("Refactor providers by protocol family"), which is the implementation of Roadmap Item #283 (priority: HIGH, status: In Progress). PR #213 also modifies http_provider.go and creates new sub-packages (openai_compat/, anthropic/) with their own type definitions.

If PR #213 merges first: Your changes here will need to be applied to the openai_compat/provider.go types as well (and the type-conversion layer in the refactored http_provider.go).

If this PR merges first: PR #213 will need to propagate ReasoningContent to all three type locations.

Recommendation: Coordinate with @jmahotiedu (PR #213 author) on merge order. Given that this is a small bug fix and #213 is a larger refactor, I'd suggest merging this first and having #213 pick up the new field.

ToolCalls []ToolCall `json:"tool_calls,omitempty"`
FinishReason string `json:"finish_reason"`
Usage *UsageInfo `json:"usage,omitempty"`
}

type UsageInfo struct {
Expand All @@ -29,10 +30,11 @@ type UsageInfo struct {
}

type Message struct {
Role string `json:"role"`
Content string `json:"content"`
ToolCalls []ToolCall `json:"tool_calls,omitempty"`
ToolCallID string `json:"tool_call_id,omitempty"`
Role string `json:"role"`
Content string `json:"content"`
ReasoningContent string `json:"reasoning_content,omitempty"` // For thinking models (e.g., GLM-Z1)
Copy link
Collaborator

@Leeaandrob Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

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

[M1 — MEDIUM: Provider-Specific Field] @siciyuan404 — The reasoning_content JSON field name is a convention used specifically by Chinese LLM providers (Zhipu GLM-Z1, Kimi K2.5, DeepSeek R1). Other providers may use different names:

  • OpenAI o1/o3: Uses a different mechanism (reasoning tokens in usage, not a content field)
  • Anthropic Claude: Uses "thinking" content blocks via the SDK

The comment says "e.g., GLM-Z1" which is accurate but incomplete. Consider updating to:

ReasoningContent string `json:"reasoning_content,omitempty"` // For thinking models (Zhipu GLM-Z1, Kimi K2.5, DeepSeek R1)

Also, if other OpenAI-compatible providers start using different field names for reasoning output, this approach won't scale. Worth adding a note in the code or docs about this limitation.

ToolCalls []ToolCall `json:"tool_calls,omitempty"`
ToolCallID string `json:"tool_call_id,omitempty"`
}

type LLMProvider interface {
Expand Down
5 changes: 3 additions & 2 deletions pkg/tools/toolloop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,9 @@ func RunToolLoop(ctx context.Context, config ToolLoopConfig, messages []provider

// 6. Build assistant message with tool calls
assistantMsg := providers.Message{
Role: "assistant",
Content: response.Content,
Role: "assistant",
Content: response.Content,
ReasoningContent: response.ReasoningContent, // Preserve for thinking models (e.g., GLM-Z1)
Copy link
Collaborator

@Leeaandrob Leeaandrob Feb 17, 2026

Choose a reason for hiding this comment

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

[Positive] @siciyuan404 — Correctly mirrored the fix from agent/loop.go. Both message-building paths (main agent loop and subagent tool loop) now preserve ReasoningContent, which means thinking models work in both direct conversations and tool-call chains. Good completeness.

}
for _, tc := range response.ToolCalls {
argumentsJSON, _ := json.Marshal(tc.Arguments)
Expand Down
Loading