Skip to content

Conversation

VisargD
Copy link
Collaborator

@VisargD VisargD commented Sep 3, 2025

Description

Motivation

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

Copy link
Contributor

Code Quality bug fix

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

  • src/providers/together-ai/chatComplete.ts: Added optional chaining (?.) to safely access parsedChunk.choices[0]?.finish_reason before calling transformFinishReason, preventing errors if choices[0] is undefined.
  • src/handlers/streamHandler.ts: Enhanced error logging by including the proxyProvider variable in console.error messages for both stream processing and writer closing failures, providing more context for debugging.

🔍 Impact of the Change

  • Improves the robustness of the Together AI chat completion stream handling by preventing potential runtime errors (TypeError) when choices[0] is missing in a stream chunk.
  • Enhances observability and debuggability for stream processing and writer closing operations by providing crucial context (proxyProvider) in error logs.

📁 Total Files Changed

  • src/handlers/streamHandler.ts: Enhanced error logging for stream processing.
  • src/providers/together-ai/chatComplete.ts: Implemented safer access to finish reason with optional chaining.

🧪 Test Added

N/A

🔒Security Vulnerabilities

N/A

Motivation

To prevent crashes due to undefined choices[0] in Together AI stream responses and to improve error diagnostics by adding proxyProvider to error logs.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

N/A

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

.

Tip

Quality Recommendations

  1. Add unit tests for together-ai/chatComplete.ts to specifically cover scenarios where parsedChunk.choices[0] might be undefined, ensuring the optional chaining works as expected.

  2. Consider implementing structured logging (e.g., using a dedicated logging library) instead of console.error for better parseability and analysis in production environments.

Tanka Poem ♫

A small fix, a gentle ?.
No more crashes, streams flow free.
Logs now richer,
Context clear, errors flee.
Code robust, for all to see. ✨

Sequence Diagram

sequenceDiagram
    participant TogetherAI as TogetherAI Stream
    participant ChatComplete as chatComplete.ts
    participant Utils as transformFinishReason

    TogetherAI->>+ChatComplete: Receive Stream Chunk (chunk)
    ChatComplete->>ChatComplete: JSON.parse(chunk) -> parsedChunk
    
    alt parsedChunk.choices[0] is defined
        ChatComplete->>+Utils: transformFinishReason(parsedChunk.choices[0].finish_reason, strictOpenAiCompliance)
        Utils-->>-ChatComplete: transformedFinishReason
        ChatComplete->>ChatComplete: Use transformedFinishReason
    else parsedChunk.choices[0] is undefined
        ChatComplete->>ChatComplete: Handle undefined choice (e.g., no finish reason)
    end
    ChatComplete-->>-TogetherAI: Processed Chunk Data
Loading

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good defensive programming fix for undefined choice access, plus improved error logging with provider context.

}
const parsedChunk: TogetherAIChatCompletionStreamChunk = JSON.parse(chunk);
const finishReason = parsedChunk.choices[0].finish_reason
const finishReason = parsedChunk.choices[0]?.finish_reason
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential runtime crash when parsedChunk.choices[0] is undefined - accessing finish_reason property on undefined would throw TypeError
Fix: Added optional chaining operator to safely access finish_reason property
Impact: Prevents runtime crashes when Together AI returns empty choices array or undefined first choice

Suggested change
const finishReason = parsedChunk.choices[0]?.finish_reason
const finishReason = parsedChunk.choices[0]?.finish_reason

}
} catch (error) {
console.error('Error during stream processing:', error);
console.error('Error during stream processing:', proxyProvider, error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Error logs lack context about which provider failed, making debugging difficult in multi-provider scenarios
Fix: Added proxyProvider parameter to error logs for better traceability
Impact: Improves debugging efficiency by identifying which provider caused stream processing failures

Suggested change
console.error('Error during stream processing:', proxyProvider, error);
console.error('Error during stream processing:', proxyProvider, error);

Comment on lines +329 to +333
console.error(
'Failed to close the writer:',
proxyProvider,
closeError
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Writer close error logs lack provider context for debugging
Fix: Added proxyProvider parameter to close error logs with proper formatting
Impact: Consistent error logging pattern across all stream handler error cases

Suggested change
console.error(
'Failed to close the writer:',
proxyProvider,
closeError
);
console.error(
'Failed to close the writer:',
proxyProvider,
closeError
);

}
} catch (error) {
console.error('Error during stream processing:', error);
console.error('Error during stream processing:', proxyProvider, error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Duplicate error logging pattern - second occurrence also lacks provider context
Fix: Added proxyProvider parameter for consistency with first error handler
Impact: Maintains consistent error logging across all stream processing error cases

Suggested change
console.error('Error during stream processing:', proxyProvider, error);
console.error('Error during stream processing:', proxyProvider, error);

Comment on lines +357 to +361
console.error(
'Failed to close the writer:',
proxyProvider,
closeError
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Second writer close error handler also lacks provider context
Fix: Added proxyProvider parameter to complete the consistent error logging pattern
Impact: All error cases now include provider information for better debugging

Suggested change
console.error(
'Failed to close the writer:',
proxyProvider,
closeError
);
console.error(
'Failed to close the writer:',
proxyProvider,
closeError
);

@VisargD VisargD merged commit 4837c8f into main Sep 3, 2025
1 check passed
@VisargD VisargD deleted the fix/together-ai-undefined-choice branch September 3, 2025 19:44
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