Skip to content

fix: preserve finish_reason in streaming final chunk#26

Merged
Kamilbenkirane merged 1 commit intomainfrom
fix/preserve-finish-reason-in-streaming
Nov 10, 2025
Merged

fix: preserve finish_reason in streaming final chunk#26
Kamilbenkirane merged 1 commit intomainfrom
fix/preserve-finish-reason-in-streaming

Conversation

@Kamilbenkirane
Copy link
Copy Markdown
Member

Preserves finish_reason in the final empty chunk of streaming responses.

Changes:

  • Track _last_finish_reason in AnthropicTextGenerationStream
  • Use _last_finish_reason instead of None in final empty chunk
  • Export TextGenerationChunk and TextGenerationFinishReason from package

Why:
Previously, the final chunk always had finish_reason=None even when the stream completed with a stop reason. This fix ensures the finish_reason is preserved and available in the final chunk.

- Track _last_finish_reason in AnthropicTextGenerationStream
- Use _last_finish_reason instead of None in final empty chunk
- Export TextGenerationChunk and TextGenerationFinishReason from package
@claude
Copy link
Copy Markdown

claude Bot commented Nov 10, 2025

PR Review: Fix preserve finish_reason in streaming final chunk

Overview

This PR addresses a bug where the finish_reason was lost in the final empty chunk of streaming responses. The fix is clean, well-scoped, and follows good practices.


✅ Code Quality & Best Practices

Strengths:

  • Minimal, focused change: Only 7 additions, 1 deletion across 2 files
  • Clear variable naming: _last_finish_reason clearly indicates purpose
  • Consistent with codebase patterns: Follows the same pattern as other provider implementations (OpenAI, Google, Cohere, Mistral all use chunks[-1].finish_reason)
  • Good PR description: Clear "why" and "what" sections
  • Proper initialization: Instance variable initialized in __init__ at streaming.py:41

Minor observations:

  • The fix is specific to Anthropic's streaming implementation, which correctly handles the unique event flow where message_delta carries the finish_reason, but message_stop doesn't

🐛 Potential Issues

No bugs identified. The implementation is sound:

  • State is properly initialized to None
  • Only set when stop_reason is present (streaming.py:212)
  • Correctly used in the final message_stop chunk (streaming.py:228)
  • The logic at streaming.py:336 (chunks[-1].finish_reason if chunks else None) will now correctly retrieve the preserved finish_reason

⚡ Performance Considerations

No performance concerns:

  • Single instance variable adds negligible memory overhead
  • No additional iterations or computations
  • State update is O(1) operation

🔒 Security Concerns

No security issues identified:

  • No user input handling in changed code
  • No external API calls modified
  • Private instance variable (underscore prefix) - proper encapsulation

🧪 Test Coverage

Gap identified:

While the fix is correct, there are no tests specifically validating this behavior:

  1. No Anthropic-specific streaming tests found in the repository
  2. General streaming tests (tests/unit_tests/test_streaming.py) test the abstract Stream class but don't cover provider-specific implementations
  3. Integration tests exist but don't assert on finish_reason in final chunks

Recommendation:
Consider adding a unit test for AnthropicTextGenerationStream that validates:

async def test_finish_reason_preserved_in_final_chunk():
    """Verify finish_reason appears in final message_stop chunk."""
    events = [
        {"type": "message_start", "message": {"content": []}},
        {"type": "content_block_delta", "delta": {"type": "text_delta", "text": "Hello"}},
        {"type": "message_delta", "delta": {"stop_reason": "end_turn"}, "usage": {...}},
        {"type": "message_stop"},  # Final chunk should have finish_reason
    ]
    # Assert final chunk has finish_reason with reason="end_turn"

This would prevent regression and document expected behavior.


📦 Public API Changes

Positive impact:

  • Exports TextGenerationChunk and TextGenerationFinishReason from package root (init.py:21-22, 30-31)
  • This is a good addition as it allows consumers to properly type-hint and inspect chunk properties
  • Maintains backward compatibility (additive change only)

Summary

Verdict: ✅ Approve with minor suggestion

The code fix is correct and well-implemented. The only improvement would be adding tests to prevent future regressions. The public API exports are a welcome addition for library consumers.

Suggested next steps:

  1. ✅ Merge (the fix is production-ready)
  2. 📝 Consider adding unit tests for Anthropic streaming finish_reason behavior (optional, can be follow-up)

Great work on identifying and fixing this edge case! 🎯

@Kamilbenkirane Kamilbenkirane merged commit 8510eff into main Nov 10, 2025
11 checks passed
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.

1 participant