Skip to content

Conversation

RomneyDa
Copy link
Collaborator

@RomneyDa RomneyDa commented Oct 18, 2025

Reverts #8280, gemini broken in main for both openai adapters and normal llm class

@RomneyDa RomneyDa requested a review from a team as a code owner October 18, 2025 09:01
@RomneyDa RomneyDa requested review from tingwai and removed request for a team October 18, 2025 09:01
Copy link

linear bot commented Oct 18, 2025

Issue reopened: CON-4266 Gemini output swallows square bracket

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Oct 18, 2025
Copy link

github-actions bot commented Oct 18, 2025

✅ Review Complete

Review of PR #8332: Revert "fix: bracket stripping in gemini responses"

Overview

This PR reverts #8280, which attempted to use SSE streaming for Gemini responses. The revert restores custom JSON array parsing logic because Gemini apparently broke in main with the SSE approach.

Critical Issues

1. Root Cause Analysis Missing (Blocker)

  • The PR description states "gemini broken in main" but doesn't explain why the SSE approach failed
  • Without understanding the failure mode, we risk reverting valid improvements or missing the actual bug
  • Recommendation: Before merging, document what specifically broke (error messages, response format differences, etc.)

2. Manual JSON Parsing is Fragile (High Priority)
The reverted code reintroduces complex manual JSON parsing with several edge cases:

if (buffer.startsWith("[")) {
  buffer = buffer.slice(1);
}
if (buffer.endsWith("]")) {
  buffer = buffer.slice(0, -1);
}
if (buffer.startsWith(",")) {
  buffer = buffer.slice(1);
}
const parts = buffer.split("\n,");

Issues:

  • Assumes specific formatting of Gemini's JSON array response
  • The split("\n,") logic is brittle - what if Gemini changes whitespace?
  • Error handling with continue; // yo! silently swallows parse errors
  • This parsing was presumably why fix: bracket stripping in gemini responses #8280 tried to use SSE in the first place

3. Testing Gap (High Priority)

  • No tests added to verify Gemini streaming works correctly after revert
  • Without tests, this could break again
  • Recommendation: Add integration tests for Gemini streaming before merging

4. Incomplete Buffer Handling
Lines in Gemini.ts around 387-390:

if (foundIncomplete) {
  buffer = parts[parts.length - 1];
} else {
  buffer = "";
}

If the incomplete part at parts[parts.length - 1] is corrupted or malformed, it accumulates indefinitely. Consider adding:

  • Maximum buffer size limit
  • Timeout for completing partial JSON

Questions

5. Was the SSE Implementation Wrong?

  • streamSse is a standard utility that should handle SSE properly
  • Does Gemini's API actually use SSE format, or is it a non-standard streaming JSON array?
  • If Gemini doesn't use SSE, the original PR fix: bracket stripping in gemini responses #8280 was fixing the wrong problem

6. VertexAI Impact
The revert affects both Gemini.ts and VertexAI.ts - have both been tested to ensure they work after this change?

Suggestions

Alternative Approach:
Instead of reverting, consider:

  1. Check if Gemini uses SSE or JSON array streaming
  2. If JSON arrays: keep the custom parser but add robust error handling
  3. If SSE: fix streamSse to handle Gemini's specific SSE format
  4. Add comprehensive tests for both paths

Monitoring:

  • Add telemetry/logging when parse errors occur
  • Track buffer sizes to detect memory issues early

Verdict

⚠️ Request Changes - The revert may be necessary, but it needs:

  1. Clear documentation of why SSE failed
  2. Tests to prevent regression
  3. Improved error handling in the manual parser
  4. Verification that both Gemini and VertexAI work correctly

The manual JSON parsing is concerning from a maintainability perspective, but if it's the only way Gemini works, at least make it robust.


Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

@sestinj sestinj merged commit 6b2edc6 into main Oct 18, 2025
57 of 58 checks passed
@sestinj sestinj deleted the revert-8280-gemini-streamsse branch October 18, 2025 15:51
@github-project-automation github-project-automation bot moved this from Todo to Done in Issues and PRs Oct 18, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Oct 18, 2025
@sestinj
Copy link
Contributor

sestinj commented Oct 18, 2025

🎉 This PR is included in version 1.27.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

released size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants