Greptile Summary

This PR fixes issue #35308 by introducing a pendingCrossTurnSeparator flag on EmbeddedPiSubscribeState that, when block streaming is active and deltaBuffer has content at the time of resetAssistantMessageState, causes a "\n\n" to be prepended to deltaBuffer on the very first handleMessageUpdate call of the new turn. This guards against cross-turn text concatenation in the streaming UI when message_end hasn't yet cleared the buffer before the next message_start fires.

However, the implementation contains a type violation: blockChunking && state.deltaBuffer.length > 0 short-circuits to undefined (not false) when blockChunking is undefined, violating the declared boolean type for pendingCrossTurnSeparator. This occurs on every non-block-streaming session. The corresponding test validates this incorrect behavior rather than catching it, asserting toBeUndefined() instead of toBe(false).

Both issues are straightforward to fix: use !!blockChunking in the production code and update the test assertion accordingly.

Confidence Score: 2/5

• Type violation on non-block-streaming sessions and misaligned test assertion create technical debt and mask the type error.
• The fix's core logic is sound and well-scoped, but it contains a type violation that breaks the type contract for EmbeddedPiSubscribeState.pendingCrossTurnSeparator on every call when block streaming is not configured. Additionally, the test explicitly validates this incorrect behavior (.toBeUndefined()) rather than catching it. While runtime behavior remains correct because undefined is falsy, the type mismatch violates the declared interface and can confuse downstream code. Both issues have straightforward fixes.
• src/agents/pi-embedded-subscribe.ts (type violation) and src/agents/pi-embedded-subscribe.handlers.messages.test.ts (incorrect assertion).

_{Last reviewed commit: 95a07ea}

Thanks for the thorough code review! I've addressed all the feedback:

Codex P1 Issues:

1. Separator preserved after trim: Now adding the separator to cleanedText instead of deltaBuffer, so it's not removed by .trim()
2. Separator set at correct point: Moved the flag setting from resetAssistantMessageState to handleMessageEnd (before deltaBuffer is cleared), ensuring the separator is properly armed

Greptile Type Issue:

• Changed from Boolean() to !! for proper boolean coercion

The key insight was that deltaBuffer gets cleared in message_end (before the next message_start), so checking deltaBuffer.length in resetAssistantMessageState would always be false. By setting the flag in handleMessageEnd (while we still have access to the content), and adding the separator to the emitted cleanedText rather than the internal buffer, we ensure the separator is preserved and delivered to streaming clients.

All tests pass and the fix has been pushed.

Thanks for the additional feedback! I've addressed the new Codex P1 issues:

P1 Issue 1: Preserve separator prefix across all chunks

• Fix: Set previousCleaned baseline to "\n\n" when adding separator
• This ensures subsequent chunks can diff correctly against the baseline that includes the separator

P1 Issue 2: Clear separator only after first visible text is emitted

• Fix: Don't clear pendingCrossTurnSeparator flag until we actually emit text
• This prevents premature consumption when the first event is thinking_*

The key insight was that the separator needs to be part of the diff baseline for all future chunks, not just the first one. By setting previousCleaned = "\n\n" when we add the separator, we ensure:

1. The delta calculation works correctly for the first chunk
2. Subsequent chunks can diff against the baseline that includes the separator
3. The monotonic guard doesn't fail

All tests pass and the fix has been pushed.

Thanks for the additional Codex review! I've addressed the P1 issue:

P1: Include separator in delta payload

• Fix: Removed the previousCleaned baseline adjustment that was causing the separator to be excluded from data.delta
• The separator now remains in both data.text and data.delta payloads

Root Cause: Setting previousCleaned = "\n\n" before computing deltaText caused the slice operation to skip the separator. By keeping previousCleaned as the actual previous value, the delta calculation correctly includes the separator.

All tests pass and the fix has been pushed.

Thanks for the additional code review! I've addressed the P2 issue:

P2: Preserve pending separator across duplicate message_end events

• Fix: Use ||= (logical OR assignment) instead of = for setting pendingCrossTurnSeparator
• This ensures the flag is preserved across duplicate message_end events
• Once set to true by the first event, duplicate events won't reset it to false

Root Cause: The original code used = which overwrites the flag on every call. With duplicate message_end events:

1. First call: deltaBuffer has content → sets flag to true, then clears deltaBuffer
2. Second call: deltaBuffer is empty → would set flag to false (losing the separator)

Solution: ||= preserves true values - once set, duplicate calls can't reset it. The flag is only consumed (reset to false) in handleMessageUpdate when the separator is actually prepended to the text.

All 6598 tests pass (6601 total, 1 unrelated locale test failed).

Co-Authored-By: Claude Sonnet 4.6 [email protected]

Thanks for the additional P2 review! I've addressed both P2 issues:

P2 #1: Preserve pending separator across duplicate message_end events

• Fix: Use ||= (logical OR assignment) to preserve the flag across duplicate events
• Once set to true, duplicate message_end events won't reset it to false

P2 #2: Base separator arming on emitted assistant text

• Fix: Change separator arming logic from deltaBuffer.length > 0 to check if this turn actually emitted content
• Now checks: deltaBuffer.length > 0 || emittedAssistantUpdate
• This handles three cases:
  1. Fully streamed turns: deltaBuffer has content → arm separator
  2. Partially streamed turns: deltaBuffer has partial content → arm separator
  3. Non-streamed turns with fallback text: emittedAssistantUpdate is true → arm separator

Root Cause: The previous logic relied solely on deltaBuffer.length, but non-streaming/late-streaming turns emit text via the fallback path at message_end without populating deltaBuffer. When combined with the ||= fix, this caused the separator flag from a previous streamed turn to be incorrectly preserved for a non-streamed turn.

Solution: Check both deltaBuffer (for streaming text) AND emittedAssistantUpdate (for fallback text emitted at message_end). This ensures the separator is armed only when the current turn actually emitted content, regardless of streaming mode.

All 11 tests pass (including 5 new tests for these fixes).

Co-Authored-By: Claude Sonnet 4.6 [email protected]

Thanks for the additional P2 review! I've addressed the issue:

P2: Arm separator only after visible output is emitted

• Fix: Remove deltaBuffer.length > 0 check, only use emittedAssistantUpdate
• This prevents arming separator for turns that only stream hidden/trimmed content

Root Cause: The previous logic checked deltaBuffer.length > 0 || emittedAssistantUpdate, but deltaBuffer contains raw chunks before stripBlockTags(...).trim() and directive parsing. This caused issues when:

• A turn streams only <thinking> tags (deltaBuffer has content, but all gets stripped)
• A turn streams only whitespace (deltaBuffer has content, but all gets trimmed)
• In these cases, emittedAssistantUpdate stays false because no visible text reaches the client, but the separator was still armed

Solution: Only check emittedAssistantUpdate, which is set to true only when visible text actually reaches the client after all processing (stripping, trimming, parsing).

Updated Logic:

// Before: armed separator for any non-empty deltaBuffer
const shouldInsertCrossTurnSeparator = !!ctx.blockChunking &&
  (ctx.state.deltaBuffer.length > 0 || ctx.state.emittedAssistantUpdate);

// After: only arm separator when visible content was emitted
const shouldInsertCrossTurnSeparator = !!ctx.blockChunking &&
  ctx.state.emittedAssistantUpdate;

This ensures the separator is armed only when real visible content was emitted, preventing spurious blank lines in the output.

All 12 tests pass (including tests for hidden tags and whitespace-only turns).

Co-Authored-By: Claude Sonnet 4.6 [email protected]

Thanks for the P1 review! I've addressed the issue:

P1: Consume separator when a turn emits only at message_end

• Fix: Add separator prepending logic to the handleMessageEnd fallback path
• This handles turns that emit text only via message_end (no text_* events)

Root Cause: The separator was only consumed in handleMessageUpdate when cleanedText was present. For non-streaming/late-streaming turns that emit text only via the handleMessageEnd fallback path:

1. Previous streamed turn arms separator
2. Next non-streamed turn emits via fallback (no text_* events)
3. handleMessageUpdate never runs with cleanedText
4. Separator is never consumed, stays pending
5. Later turn gets spurious separator

Solution: The fallback path now also:

• Checks pendingCrossTurnSeparator
• Prepends "\n\n" to cleanedText if separator is pending
• Clears the flag

Code Changes:

// In handleMessageEnd fallback path (line 322-341):
let finalText = cleanedText;
if (ctx.state.pendingCrossTurnSeparator && finalText) {
  finalText = "\n\n" + finalText;
  ctx.state.pendingCrossTurnSeparator = false;
}

This ensures the separator is consumed whether the turn emits via streaming updates or via the message_end fallback path.

All 15 tests pass (including 3 new tests for fallback path separator consumption).

Co-Authored-By: Claude Sonnet 4.6 [email protected]