fix: complete reasoning passback + fix stripGarbledToolXML discarding…#1
fix: complete reasoning passback + fix stripGarbledToolXML discarding…#1pragan333 wants to merge 1 commit into
Conversation
… valid content - Copy resp.Thinking to assistant message in agent loop so reasoning_content is actually passed back to thinking models (Kimi, DeepSeek) on multi-turn - Fix stripGarbledToolXML returning empty string when valid content remains after stripping XML artifacts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes two bugs in the agent loop:
- Reasoning passback fix (
loop.go): The assistant message built during the tool-call loop was missingresp.Thinking, so thinking models (Kimi, DeepSeek) never received theirreasoning_contentechoed back in subsequent loop turns — breaking multi-turn coherence for those models. stripGarbledToolXMLfix (sanitize.go): The function previously discarded all content (return "") even when valid non-XML text remained after stripping garbled XML artifacts. The fix returns the stripped-clean remaining text instead, along with a more descriptive warning log message.
Changes:
- Propagate
resp.Thinkinginto the assistant message during the tool-call loop to enablereasoning_contentpassback for thinking models on multi-turn conversations - Fix
stripGarbledToolXMLto return remaining content instead of an empty string when valid text survives the XML stripping
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/agent/loop.go |
Adds Thinking: resp.Thinking to the assistant message struct in the tool-call loop, enabling reasoning content passback |
internal/agent/sanitize.go |
Fixes stripGarbledToolXML to return remaining non-XML content rather than silently discarding it; updates the log message to reflect the new behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if cleaned != "" && hasIndicator { | ||
| slog.Warn("stripped garbled tool call response", | ||
| slog.Warn("stripped garbled tool call XML, preserving remaining content", | ||
| "original_len", len(content), | ||
| "remaining_len", len(cleaned), | ||
| ) | ||
| return "" | ||
| return cleaned |
There was a problem hiding this comment.
The new behavior of stripGarbledToolXML — returning cleaned when valid content remains after removing XML artifacts — is not covered by any tests. The internal/agent package has test files for other modules (input_guard_test.go, loop_history_test.go, toolloop_test.go), but sanitize.go has no dedicated test file at all.
This is a behavior-changing fix (previously returned "", now returns the remaining text), and it would benefit from test cases covering at minimum:
- A response that contains only garbled XML → should return
"" - A response with garbled XML plus real text → should return the real text
- A response with no garbled XML → should be returned unchanged
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ccf40188b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… valid content