Skip to content

fix(openai): merge ThinkingBlock and tool reasoning details instead of overwriting - #2914

Open
chaucerj wants to merge 12 commits into
agentscope-ai:mainfrom
chaucerj:fix/reasoning-details-overwrite
Open

fix(openai): merge ThinkingBlock and tool reasoning details instead of overwriting#2914
chaucerj wants to merge 12 commits into
agentscope-ai:mainfrom
chaucerj:fix/reasoning-details-overwrite

Conversation

@chaucerj

@chaucerj chaucerj commented Aug 31, 2026

Copy link
Copy Markdown

Fixes #2913

What

OpenAIMessageConverter.convertAssistantMessage called builder.reasoningDetails(...) twice: once for details restored from the ThinkingBlock metadata, once for details collected from tool use metadata. The second call silently overwrote the first, so an assistant message containing both a thinking block and tool calls lost the thinking-side reasoning details.

Change

Collect both sources into a single list and set reasoningDetails exactly once, after the tool-call section:

  • hoist List<OpenAIReasoningDetail> reasoningDetails above the ThinkingBlock handling
  • ThinkingBlock path adds to that list instead of calling the builder
  • tool-call path reuses the same list (local declaration removed)
  • one builder.reasoningDetails(reasoningDetails) after tool calls, covering the no-tool-calls case too

Test

testThinkingDetailsMergedWithToolDetails asserts a message with both a ThinkingBlock (METADATA_REASONING_DETAILS) and a tool call (reasoningDetail metadata) yields both details ([thinking-detail, tool-detail]); before the fix it returned only [tool-detail]. Existing thinking-only / tool-only tests unchanged and green.

Note

Stacked on #2910 (same file, adjacent code). Merge #2910 first; this PR's own commit is the last one.

🤖 Generated with Claude Code

chaucerj and others added 6 commits August 30, 2026 21:52
@chaucerj @claude
fix(model): preserve thought signatures across JSON persistence round…
442b02a 
…-trips

Gemini 3 requires thought signatures to be replayed with function calls.
They are stored as byte[] in ToolUseBlock metadata, but a JSON persistence
round-trip (session save/load) restores them as a Base64 String, so the
converters' instanceof checks silently dropped them and the next request
failed with 400 INVALID_ARGUMENT from the server.

- GeminiMessageConverter: decode Base64 String signatures back to byte[]
- OpenAIMessageConverter: normalize byte[] signatures to Base64 String
  (covers the in-memory cross-provider path) and restore reasoning detail
  metadata that comes back from persistence as a LinkedHashMap

Verified against the live gemini-3-flash-preview API: replaying a
function call without its signature returns 400 "Function call is
missing a thought_signature in functionCall parts"; replaying it with
the signature returns 200.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
ci: retrigger workflows after flaky harness test
515750a 
HarnessAgentDynamicHookBuilderTest failed once in agentscope-harness
(own surefire JVM, unrelated to this change; another PR hit the same
1-error-in-844 flake on a different harness test the same day). No code
change.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
test(model): cover tool use blocks without metadata
455afd4 
Adds a no-metadata ToolUseBlock case to both converter test classes,
exercising the null-metadata guards left as partial branches by the
previous coverage pass. Patch coverage is now 100% of changed lines.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
test(model): cover remaining partial branches in signature handling
e3dda89 
ToolUseBlock normalizes null metadata to an empty map, so the null-side
of the metadata guards is unreachable and cannot be covered. The actual
partial branches were:

- GeminiMessageConverter: a signature value that is neither byte[] nor
  String (now covered by a non-string signature test)
- OpenAIMessageConverter: a ThinkingBlock details list whose entries
  cannot be restored (now covered), and a dead !candidate.isEmpty()
  check in the fallback-signature scan - toSignatureString never returns
  an empty string, so the check is dropped.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
fix(openai): merge ThinkingBlock and tool reasoning details instead o…
ab46fd9 
…f overwriting

convertAssistantMessage called builder.reasoningDetails(...) twice — once for
details restored from ThinkingBlock metadata and once for tool-call details.
The second call silently replaced the first, so assistant messages carrying
both a thinking block and tool calls dropped the thinking-side reasoning
details (issue agentscope-ai#2913).

Collect both sources into one list and set reasoningDetails once, after the
tool-call section.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
ci: re-run tests
d757cc8 
The ubuntu build failed in agentscope-harness JsonSessionDefaultLocationTest
@tempdir cleanup (DirectoryNotEmptyException during teardown, assertions all
passed) - unrelated to this change; re-triggering.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj

chaucerj commented Aug 31, 2026

Copy link
Copy Markdown
Author

The two red ubuntu runs are both unrelated to this change:

  1. Run 33346235574 failed in agentscope-harness JsonSessionDefaultLocationTest — a @TempDir teardown DirectoryNotEmptyException (async session writer leaves bob/s1 dirs behind); the assertions themselves passed.
  2. Run 33346777378 fails to compile agentscope-extensions-rag-simple test sources: apiKey(any()) is ambiguous because main now pins openai-java 4.52.0, which added apiKey(Optional<String>). main itself is currently red for the same reason (run 33321234498) — fixed by fix(rag): resolve ambiguous apiKey overload in OpenAITextEmbeddingEmbedTest after openai-java 4.52.0 upgrade #2912.

This branch only touches OpenAIMessageConverter + its test. Once #2912 lands I'll re-trigger.

chaucerj and others added 6 commits August 31, 2026 10:00
@chaucerj @claude
Merge remote-tracking branch 'origin/main' into fix/thought-signature…
27634e2 
…-persistence

Sync with latest main (f96cdc7), including the openai-java 4.52.0 bump.
No code changes; merge was conflict-free.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
Merge fix/thought-signature-persistence (synced with main) into fix/r…
899b77a 
…easoning-details-overwrite

Pull in the main sync merge; keeps this branch stacked on the updated
fix/thought-signature-persistence. No code changes.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
Merge remote-tracking branch 'origin/main' into fix/thought-signature…
c60137e 
…-persistence

Sync with main (b436110): picks up the rag-simple apiKey ambiguity fix
(agentscope-ai#2912) so CI can go green again. No code changes of our own.
Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
Merge fix/thought-signature-persistence (synced with main) into fix/r…
810d543 
…easoning-details-overwrite

Pull the main sync (b436110, includes rag-simple CI fix agentscope-ai#2912) into this
stack. No code changes of our own.
Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
ci: re-run tests after Windows @tempdir flake
c1894b3 
HarnessAgentDynamicHookBuilderTest failed on windows-latest with a
suppressed DirectoryNotEmptyException in @tempdir teardown (assertions
passed). Unrelated to this change; re-running.
Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj @claude
ci: re-run tests after harness @tempdir flake (again)
a699daf 
Third occurrence of the same harness flake class: async session writer
leaves subdirs behind and @tempdir teardown throws (suppressed
DirectoryNotEmptyException; assertions pass, 0 failures). Not related to
this change.
Co-Authored-By: Claude Code <noreply@anthropic.com>
@chaucerj

chaucerj commented Aug 31, 2026

Copy link
Copy Markdown
Author

The latest failures on this PR are all the same flaky-test class in the harness module, unrelated to this change (which only touches OpenAIMessageConverter and its tests):

Run OS Test Error
33356275214 windows HarnessAgentDynamicHookBuilderTest.disableDynamicSkills_skillsEnabledFalseLeavesCatalogEmpty suppressed DirectoryNotEmptyException
33357579563 ubuntu HarnessAgentIntegrationExampleTest.example_fullWorkspace_singleTurn_seesSessionSubagentsAndWorkspaceContext suppressed DirectoryNotEmptyException (leftover integration-session-1/)
33358003309 ubuntu HarnessAgentDynamicHookBuilderTest.disableDynamicSkills_skillsEnabledFalseLeavesCatalogEmpty suppressed DirectoryNotEmptyException

In every case: assertions pass (Failures: 0), and the error is thrown from JUnit @TempDir teardown as a suppressed DirectoryNotEmptyException — a background session writer appears to still hold/leave subdirectories when the temp dir is being cleaned up.

Evidence it's not this PR: the sibling PR #2910 (identical tree except this PR's 17-line converter change, which no harness test routes through) passed the same suite green on both OSes in run 33356276237.

I lack admin rights to re-run failed jobs, so I re-triggered via empty commits twice; the flake hit a third time. Maintainers with admin should be able to get it green with a simple "Re-run failed jobs". Longer term it may be worth making the session writer await termination before test teardown.

oss-maintainer
oss-maintainer approved these changes Sep 3, 2026
View reviewed changes

@oss-maintainer oss-maintainer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. fix(openai): merge ThinkingBlock and tool reasoning details instead of overwriting — change looks correct and clean.

Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oss-maintainer oss-maintainer oss-maintainer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: OpenAIMessageConverter drops ThinkingBlock reasoning details when the message also contains tool calls (tool-call details overwrite them)

2 participants

@chaucerj @oss-maintainer