Repository navigation
fix(providers): coalesce consecutive Thinking blocks in collect_stream - #11317
Conversation
Conversation::push already coalesces consecutive thinking deltas by chunk id (added in aaif-goose#9162), but collect_stream — the default path every Provider::complete() call goes through — only had a coalescing arm for Text blocks. Thinking deltas fell through to the wildcard arm and were persisted one streamed chunk per content block. The effect is purely a storage/serialization blow-up, not a correctness or token-cost issue: formats/openai.rs concatenates thinking blocks with no separator, so N fragments and one merged block produce byte-identical model input, and token_counter never counts Thinking at all. But on a real session (ollama + qwen3.6, tool-pair compaction path), 12,632 fragmented content items collapsed to 249 once coalesced — a 50x reduction in content_json bytes. Add the same signature-aware merge rule collect_stream's Text arm already follows for Text: mirror Conversation::push's rule exactly (append while the previous block is unsigned or shares the incoming signature; never merge two distinctly-signed blocks). Includes the regression test from the issue plus a signature-boundary test. Fixes aaif-goose#11305 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2034121e6
ℹ️ 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".
| ) if last_thinking.signature.is_empty() | ||
| || new_thinking.signature == last_thinking.signature => |
There was a problem hiding this comment.
Preserve multi-block Thinking boundaries
When a stream chunk contains more than one content block and starts with Thinking, this arm now merges that block into the previous Thinking whenever the previous signature is empty or matches. Conversation::push deliberately applies the analogous signature rule only for single-content delta messages, so a multi-block message can remain a complete block plus adjacent content. For inputs like an unsigned previous Thinking followed by [Thinking("next", "sig"), Text(...)], collect_stream will produce one signed thinking block covering both texts, corrupting the thinking/signature boundary; please gate this merge on the incoming message having exactly one content block or otherwise preserve multi-block boundaries.
Useful? React with 👍 / 👎.
DOsinga
left a comment
There was a problem hiding this comment.
The core merge rule is correct for single-content deltas, but this does not yet mirror Conversation::push for multi-content chunks.
collect_stream iterates each block of an incoming message and merges its first Thinking block into the prior message regardless of how many blocks the incoming message contains. Conversation::push intentionally requires message.content.len() == 1. For an unsigned prior thinking delta followed by a chunk shaped [Thinking("next", "sig"), Text(...)], this implementation signs the concatenation of the prior and new thinking text, erasing a block/signature boundary. Please retain the incoming content count and gate thinking coalescing on a single-content incoming message (or otherwise preserve the same boundary semantics).
Please also add direct collect_stream tests for: (1) unsigned body + signed closing delta adopts the signature, (2) signed thinking followed by unsigned thinking remains separate, and (3) a multi-content incoming message does not merge its thinking block into the prior block. The existing tests cover the basic unsigned case and two distinct non-empty signatures, but not the asymmetric signature-at-end cases or the multi-block constraint that exposed this issue.
Addresses review from DOsinga and the automated Codex reviewer on aaif-goose#11317: the Thinking merge arm was firing on the first block of any incoming message, including multi-block chunks. Conversation::push gates its equivalent merge on the incoming message having exactly one content block, so a chunk that bundles a complete (signed) Thinking block with subsequent content is treated as a structured unit, not a raw delta. collect_stream's per-block loop merged the first item of such a chunk into the previous message's last block regardless, which for an unsigned prior block followed by [Thinking(t, sig), ...] signed the concatenation of the prior and new thinking text, erasing the block/signature boundary. Capture msg.content.len() before the loop and gate the Thinking arm on it being exactly 1, mirroring Conversation::push precisely. Add the three tests DOsinga requested: unsigned body adopting a closing signature, an unsigned delta after a signed block starting a new block (not merging into the closed one), and the multi-block case that exposed the bug — verified this last test fails with the old unguarded merge (reproduces the exact "priornext"/signed corruption from the review) and passes with the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Confirmed and fixed — reproduced the exact corruption first (stashed the guard, ran the multi-block test: Added the three tests you asked for: unsigned body adopting a closing signature, an unsigned delta after a signed block starting a new block, and the multi-block case that exposed the bug (verified failing without the fix, passing with it).
— Assisted by Claude · Approved by James Wolfe (@Wolfe-Jam) |
* main: (70 commits) cli: remove recipe secret discovery (#11435) fix(openrouter): escape Gemini tool response ref keys (#11276) fix(security): honor MCP tool model visibility in Code Mode (#11425) fix(providers): estimate cost for Azure Foundry models via inferred catalog pricing (#11264) feat(providers): add Gondola as declarative OpenAI-compatible provider (#11421) feat(otel): add request params, response metadata, tool call parity, and agent identification (#11261) fix(providers): coalesce consecutive Thinking blocks in collect_stream (#11317) feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (#11120) add MCP conformance tests to goose CI (combines #10800 + #10801) (#10940) feat(desktop): sort configured providers to the top of the provider list (#11409) fix(cli): refuse symlink diagnostics outputs (#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (#11407) fix(config): serialize secret mutations (#11388) fix: decouple source file and tool response limits (#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (#11245) fix(security): suppress sensitive OTLP traces (#11381) feat(openrouter): forward session_id and add app category header (#10868) feat(acp): derive and forward thinking effort from the ACP harness (#10949) fix(aws_bedrock): replace flat model list with routing table, add Gemma 4 Mantle support (#10297) Add GPT-5.6 follow-up support for Codex and Responses API (#10460) ...
* main: (107 commits) fix(providers): inform user of clipboard copy and remove copilot auth retry on timeout (aaif-goose#11160) feat(desktop): select saved recipes when creating a schedule (aaif-goose#10892) More provider test scripts (aaif-goose#10515) cli: remove recipe secret discovery (aaif-goose#11435) fix(openrouter): escape Gemini tool response ref keys (aaif-goose#11276) fix(security): honor MCP tool model visibility in Code Mode (aaif-goose#11425) fix(providers): estimate cost for Azure Foundry models via inferred catalog pricing (aaif-goose#11264) feat(providers): add Gondola as declarative OpenAI-compatible provider (aaif-goose#11421) feat(otel): add request params, response metadata, tool call parity, and agent identification (aaif-goose#11261) fix(providers): coalesce consecutive Thinking blocks in collect_stream (aaif-goose#11317) feat(hooks): add PreToolUseResult event and stable tool_call_id across tool lifecycle (aaif-goose#11120) add MCP conformance tests to goose CI (combines aaif-goose#10800 + aaif-goose#10801) (aaif-goose#10940) feat(desktop): sort configured providers to the top of the provider list (aaif-goose#11409) fix(cli): refuse symlink diagnostics outputs (aaif-goose#11398) test(plugins): isolate GOOSE_PATH_ROOT in discovery tests (aaif-goose#11407) fix(config): serialize secret mutations (aaif-goose#11388) fix: decouple source file and tool response limits (aaif-goose#11391) chore(deps): bump pctx_code_mode from 0.4.1 to 0.5.0 (aaif-goose#11245) fix(security): suppress sensitive OTLP traces (aaif-goose#11381) feat(openrouter): forward session_id and add app category header (aaif-goose#10868) ...
aaif-goose#11317) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 1f6c752) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VvuBB2cVVZ8Herdq3zyuMQ
aaif-goose#11317) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit fe51be3) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VvuBB2cVVZ8Herdq3zyuMQ
Fixes #11305
Summary
Conversation::pushalready coalesces consecutive thinking deltas by chunk id (added in #9162), butcollect_stream— the default path everyProvider::complete()call goes through — only has a coalescing arm forTextblocks.Thinkingdeltas fall through to the wildcard arm and are persisted one streamed chunk per content block.This is a storage/serialization defect, not a correctness or token-cost one:
formats/openai.rsconcatenates thinking blocks with no separator, so N fragments and one merged block produce byte-identical model input, andtoken_counternever countsThinkingat all. But on a real session it's a real blow-up — the issue measured 12,632 fragmented content items collapsing to 249 once coalesced, a 50x reduction incontent_jsonbytes.Adds the same signature-aware merge rule
collect_stream'sTextarm already applies, mirroringConversation::push's rule exactly: append while the previous block is unsigned or shares the incoming signature; never merge two distinctly-signed blocks.Testing
cargo test -p goose-provider-types --lib— 527 passed, 0 failedcargo clippy -p goose-provider-types --all-targets— clean, 0 warningscargo fmt --check— cleancargo check -p goose— downstream crate (where the reported real-world impact lives, via tool-pair compaction) compiles clean against the changetest_collect_stream_coalesces_thinking_deltas(the regression test from the issue, verbatim) andtest_collect_stream_never_merges_distinctly_signed_thinking_blocks(the signature-boundary edge case from the expected-behavior spec)