Skip to content

fix: unpack StreamingThinkScrubber tuple in streaming delta paths - #37382

Closed
lld1995 wants to merge 1 commit into
NousResearch:mainfrom
lld1995:fix/think-scrubber-tuple
Closed

fix: unpack StreamingThinkScrubber tuple in streaming delta paths#37382
lld1995 wants to merge 1 commit into
NousResearch:mainfrom
lld1995:fix/think-scrubber-tuple

Conversation

@lld1995

@lld1995 lld1995 commented Jun 2, 2026

Copy link
Copy Markdown

Every API call through the chat-completions transport failed with can only concatenate str (not tuple) to str. StreamingThinkScrubber.feed()/flush() return a (visible, reasoning) tuple but two call sites in run_agent.py assigned the whole tuple to text; the tuple later hit a str+ concat. Fixed by unpacking at _fire_stream_delta (hot path) and _reset_stream_delivery_tracking (end-of-stream flush). Verified locally and against the updated container image.

…eset_stream_delivery_tracking

StreamingThinkScrubber.feed() and flush() both return a (visible, reasoning)
tuple, but two call sites assigned the entire tuple back to the 'text' variable,
causing 'can only concatenate str (not "tuple") to str' on every API call.

- _fire_stream_delta (hot path, every streaming delta)
- _reset_stream_delivery_tracking (end-of-stream flush)
@liuhao1024

Copy link
Copy Markdown
Contributor

This PR unpacks think_scrubber.flush() and think_scrubber.feed() as returning (visible, reasoning) tuples, but on current main both methods return str, not a tuple:

# agent/think_scrubber.py on main:
def feed(self, text: str) -> str:     # line 106
def flush(self) -> str:               # line 204

Merging this PR without the scrubber change would cause ValueError: not enough values to unpack at runtime on both call sites.

I see that your PR #23638 modifies agent/think_scrubber.py to return tuples — this PR appears to be stacked on top of that one. The diff alone doesn't include the scrubber change, so this would break if merged independently.

Suggestion: either:

  1. Merge feat(api_server): stream inline <think> as delta.reasoning_content (unify A/B-class reasoning) #23638 first, then this PR can land cleanly, or
  2. Combine both PRs into a single self-contained PR so reviewers can see the full picture (scrubber return type change + call-site unpacking).

Also, the PR body says "StreamingThinkScrubber.feed()/flush() return a (visible, reasoning) tuple" as if it's current behavior — but it's not on main. Adding a "depends on #23638" note would help reviewers understand the stacking.

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jun 2, 2026
@lld1995

lld1995 commented Jun 2, 2026

Copy link
Copy Markdown
Author

Closing this PR. As the reviewer correctly pointed out, this change only includes the run_agent.py call-site unpacking but not the agent/think_scrubber.py return-type change (on main, feed()/flush() still return str, so merging this alone would raise ValueError: not enough values to unpack at runtime).

The fix is already fully and self-containedly covered by #23638, which changes think_scrubber.py to return (visible, reasoning) tuples and updates both call sites in run_agent.py (_reset_stream_delivery_tracking + _fire_stream_delta) plus the api_server.py reasoning channel and tests. Consolidating there instead of stacking. Thanks for catching this!

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants