Skip to content

fix: show reasoning on the reasoning stream so thoughts render - #868

Merged
will-lamerton merged 3 commits into
Nano-Collective:mainfrom
addyCooks:fix/empty-thought-dropdown
Aug 21, 2026
Merged

fix: show reasoning on the reasoning stream so thoughts render#868
will-lamerton merged 3 commits into
Nano-Collective:mainfrom
addyCooks:fix/empty-thought-dropdown

Conversation

@addyCooks

@addyCooks addyCooks commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Closes #853.

Description

The thought dropdown rendered but expanded to nothing. Three changes:

  • source/ai-sdk-client/chat/chat-handler.ts streamed tokens are batched behind a 150ms timer, but the reasoning/text routing flag was driven by the *-start / *-end markers around a batch rather than by the deltas that filled it. The delta type is now the source of truth: reasoning-delta / text-delta flush the pending batch before flipping the flag, and the four start/end cases collapse into a single flush-only case. Routing is now independent of provider ordering.
  • source/acp/acp-conversation.ts / acp-agent.ts leading whitespace-only reasoning no longer emits an agent_thought_chunk, is no longer stored on the message, and is skipped on session-history replay. The callback accumulates only what it emits, so replaySessionHistory re-sending the stored value renders identically to the live stream.
  • plugins/vscode/media/chat-panel.js a thought section is only opened once there is non-whitespace text, and endCurrentTextBlock() moved inside that guard so an empty chunk can't split the answer.

Upstream references for the orderings this has to survive: @ai-sdk/openai emits reasoning-end on part.done only when store is set, otherwise it defers to output_item.done; @ai-sdk/openai-compatible emits reasoning-start without ever closing an active text part. Some providers emit reasoning deltas with no start marker at all.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API driven end-to-end against a local stub OpenAI-compatible SSE endpoint (real --acp server → real chat-panel.js), not a hosted provider; see the review reply for exactly what was covered
  • Tested MCP integration (if applicable)

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging

@will-lamerton

Copy link
Copy Markdown
Member

Hey @addyCooks - nice diagnosis - the batch-flush ordering is the real root cause, and the chat-handler tests genuinely reproduce it (the deltas are enqueued synchronously, so flushTimer is still pending when the flip arrives, which is exactly the failing condition). flushPending() also cleans up three copies of clear-then-flush. One thing I'd like changed before merge, plus a few smaller items.

Requested change

Routing is still driven by start/end markers rather than by the delta type.

isReasoning is only ever assigned in the *-start / *-end cases, so any reasoning-delta arriving while isReasoning === false still lands on onToken. That happens whenever a provider emits reasoning deltas without a preceding reasoning-start, or interleaves reasoning deltas after text-start because reasoning-end was deferred - which is precisely the ordering unreliability the PR description cites as the root cause. The fix hardens the flush but leaves the flag's source of truth unchanged, so the bug can still recur on a provider whose ordering diverges from the two cases covered by the tests.

Deriving the flag from the delta itself makes it ordering-independent and reduces the start/end handlers to pure flush points:

case 'reasoning-delta':
	if (!isReasoning) {
		flushPending();
		isReasoning = true;
	}
	accumulatedReasoning += chunk.text;
	tokenBuffer += chunk.text;
	...
case 'text-delta':
	if (isReasoning) {
		flushPending();
		isReasoning = false;
	}
	...

This also neutralises isReasoning = false on reasoning-end, which is currently a small latent hazard if a provider emits further reasoning deltas after closing the item. Worth a test with a bare reasoning-delta (no reasoning-start) to lock it in.

Smaller items

Live and replay disagree on leading whitespace. acp-conversation.ts:151 appends the token to streamedReasoning before the early return, so the raw value including the leading \n\n is what gets stored at line 244. replaySessionHistory then re-sends message.reasoning verbatim once it passes the new .trim().length > 0 guard. The same session renders as "foo" live and "\n\nfoo" after loadSession. Either trimStart() on the replay's text: field, or store the trimmed value.

Whitespace-only reasoning is still persisted. reasoning: streamedReasoning || undefined (acp-conversation.ts:244) keeps "\n\n" on the message even though nothing was emitted for it. streamedReasoning.trim() ? streamedReasoning : undefined stops it at the source and leaves the acp-agent guard as backwards-compat for sessions already on disk.

The user-visible piece has no coverage. plugins is excluded from Biome (biome.json:18) and AVA only globs plugins/**/*.spec.ts, so nothing lints or exercises plugins/vscode/media/chat-panel.js. The guard itself reads correctly - thoughtText && (currentThoughtBox || thoughtText.trim()) opens on first non-whitespace and keeps appending afterwards, and moving endCurrentTextBlock() inside the guard is an improvement over the old unconditional call. But every "Manual Testing" box is unchecked, and the dropdown rendering is the reported symptom, so could you do one manual pass in the extension and tick the relevant box?

Test shape nit. acp-conversation.spec.ts invokes capturedCallbacks.onReasoningToken(...) after runAcpConversation has already resolved. It passes because the closure over streamedReasoning outlives the turn, but it is asserting against a finished turn, so it would not catch a regression that moved the per-turn let streamedReasoning = '' reset out of the loop.

@addyCooks
addyCooks force-pushed the fix/empty-thought-dropdown branch from 40c6e27 to 19c81a9 Compare August 20, 2026 20:48
@addyCooks

addyCooks commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @will-lamerton all four addressed in c2e8e11. Rebased onto current main.

Routing by delta type. reasoning-delta / text-delta now own the flag (flush, then flip); the four start/end cases collapse into one flush-only case, so isReasoning = false on reasoning-end is gone. Three new tests, each verified to fail on the marker-driven version including the bare-delta case you asked for and the post-reasoning-end hazard.

Whitespace. Fixed at the source instead of on replay: the callback accumulates only what it emits (streamedReasoning ? token : token.trimStart()), so the stored value is exactly the thought chunks and replaySessionHistory can keep re-sending it verbatim. A test asserts assistant.reasoning === thoughtTexts(updates).join(''). Plus streamedReasoning.trim() ? … : undefined as you wrote it; the acp-agent guard stays, commented as back-compat.

chat-panel.js coverage. Your premise was right when you wrote it, but main has since picked up chat-panel-thoughts.spec.ts + chat-panel-harness.ts, which run the real panel in a VM sandbox — and under source/, AVA does glob it. Four tests added there, all failing without the guard, including one that locks in the endCurrentTextBlock() move.

Test shape. Tokens now stream from inside chat(), so assertions run against a live turn. Added resets streamed reasoning between turns, the regression the old shape couldn't catch.

Manual pass worth being precise: not a live extension session. I drove the real --acp server against a stub OpenAI-compatible SSE endpoint and replayed the captured session/updates through the real chat-panel.js. The deferred-reasoning-end ordering reproduces #853's screenshot exactly against main:

BEFORE  thought: "<md>\n\n</md>"   ← "Thought for 0s", renders to nothing
        answer:  "The user wants the capital of France. That is Paris.The capital of France is Paris."

AFTER   thought: "The user wants the capital of France. That is Paris."
        answer:  "The capital of France is Paris."

So the thoughts were never lost they were printing as the assistant's reply, and the empty bubble was the leading \n\n opening a section on its own.

I ticked Tested with OpenAI-compatible API on that basis with an inline caveat. Happy to untick if you'd rather that box mean a live session only that pass is still outstanding.

CI is green, full suite included.

addyCooks and others added 2 commits August 21, 2026 02:21
Streamed tokens are batched behind a 150ms timer, but flushBuffer read
isReasoning at flush time instead of at buffer time. A provider that opens
the next stream inside that window — the Responses API defers reasoning-end
until the reasoning item completes, and openai-compatible reopens reasoning
without closing text — flipped the flag first, so the batch went out on the
wrong callback: reasoning surfaced as assistant text and the thought view
stayed empty.

Flush pending tokens before the switch, and drop whitespace-only reasoning
from ACP thought chunks and from the webview's thought section, since marked
renders it to nothing and leaves a bare "Thought for 0s" bubble behind.
Review follow-up to the batch-flush fix. isReasoning was still only
assigned in the *-start / *-end cases, so a reasoning delta arriving with
no preceding reasoning-start - or after a provider deferred reasoning-end
past text-start - kept landing on onToken. The delta type is now the
source of truth and the four start/end cases collapse into a single flush
point, which makes routing independent of provider ordering and removes
the isReasoning = false on reasoning-end.

The ACP reasoning callback now accumulates only what it emits, so the
stored reasoning is exactly what the live stream showed and
replaySessionHistory can re-send it verbatim; whitespace-only reasoning is
no longer persisted on the message either. The webview whitespace guard
gains coverage in source/vscode/chat-panel-thoughts.spec.ts, the harness
main picked up since this branched.
@addyCooks
addyCooks force-pushed the fix/empty-thought-dropdown branch from 19c81a9 to c2e8e11 Compare August 20, 2026 21:03
@will-lamerton
will-lamerton merged commit 281874e into Nano-Collective:main Aug 21, 2026
10 of 11 checks passed
@will-lamerton

Copy link
Copy Markdown
Member

Nice work @addyCooks :)

@addyCooks

Copy link
Copy Markdown
Contributor Author

Thanks @will-lamerton! : )

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] thoughts are not visible in the thought dropdown

2 participants