Skip to content

fix(sse): track lastStatus on quality-check failure and detect SSE event: prefix - #1440

Closed
benzntech wants to merge 2 commits into
diegosouzapw:release/v3.7.0from
benzntech:fix/combo-quality-check-laststatus
Closed

benzntech wants to merge 2 commits into
diegosouzapw:release/v3.7.0from
benzntech:fix/combo-quality-check-laststatus

Conversation

@benzntech

Copy link
Copy Markdown
Contributor

Problem

Two bugs in the combo routing engine caused false ALL_ACCOUNTS_INACTIVE errors:

Bug 1: lastStatus tracking gap in quality-check failure path

When all combo models fail via the quality-check path (200 response that fails validateResponseQuality), lastStatus was never set because result.ok is true. The code only sets lastStatus in the !result.ok error branch. This caused the final check if (!lastStatus) to return ALL_ACCOUNTS_INACTIVE instead of the proper error status.

Bug 2: SSE event: prefix not recognized

validateResponseQuality only recognized SSE payloads starting with data:. Anthropic-style SSE starts with event: lines before data: lines, so non-streaming requests routed through anthropic-compatible-* providers that returned streaming responses were falsely rejected as "response is not valid JSON".

Fix

combo.ts (2 changes):

  1. Set lastStatus = 422 and lastError in the quality-check failure path (both priority and round-robin code paths), so the combo returns proper 422 status with the quality-check failure reason instead of ALL_ACCOUNTS_INACTIVE
  2. Accept SSE responses starting with event: or containing \ndata: in addition to data: in validateResponseQuality

Tests

3 new tests in combo-routing-engine.test.ts:

  • handleComboChat returns 422 (not ALL_ACCOUNTS_INACTIVE) when all models fail quality check
  • handleComboChat accepts SSE responses starting with event: line as valid
  • handleComboChat accepts SSE responses with event and data on separate lines

All 55 combo routing tests pass (52 existing + 3 new).

Auto-syncs main branch from upstream diegosouzapw/OmniRoute
every 6 hours. Also triggerable manually via workflow_dispatch.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@benzntech
benzntech requested a review from diegosouzapw as a code owner April 20, 2026 04:35
…ent: prefix

When all combo models fail via the quality-check path (200 response
with invalid body), lastStatus was never set because result.ok is true.
This caused the combo to return ALL_ACCOUNTS_INACTIVE instead of the
proper error status.

Additionally, validateResponseQuality only recognized SSE payloads
starting with "data:" but not "event:" lines (Anthropic-style SSE),
causing false quality-check failures for non-streaming requests routed
through anthropic-compatible providers that return streaming responses.

Fixes:
- Set lastStatus=422 and lastError in quality-check failure path
  (both priority and round-robin code paths)
- Accept SSE responses starting with "event:" or containing
  "\ndata:" in addition to "data:"
@benzntech
benzntech force-pushed the fix/combo-quality-check-laststatus branch from 3397f90 to 06fcd3c Compare April 20, 2026 04:36

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request enhances SSE response validation by supporting 'event:' prefixes and ensures that quality check failures correctly set a 422 status code and error message. Feedback was provided to update the error handling logic so that the most recent failure reason is captured, maintaining consistency with existing patterns in the codebase.

});
recordedAttempts++;
if (!lastStatus) lastStatus = 422;
lastError = lastError || `quality check failed: ${quality.reason}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To maintain consistency with the error handling logic in the !result.ok branch (see line 1705), lastError should be updated with the latest failure reason rather than preserving the first one. The current use of the logical OR operator (||) deviates from the established pattern where lastStatus captures the first status code while lastError captures the most recent error message.

Suggested change
lastError = lastError || `quality check failed: ${quality.reason}`;
lastError = `quality check failed: ${quality.reason}`;

});
recordedAttempts++;
if (!lastStatus) lastStatus = 422;
lastError = lastError || `quality check failed: ${quality.reason}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To maintain consistency with the error handling logic in the !result.ok branch (see line 2053), lastError should be updated with the latest failure reason rather than preserving the first one. The current use of the logical OR operator (||) deviates from the established pattern where lastStatus captures the first status code while lastError captures the most recent error message.

Suggested change
lastError = lastError || `quality check failed: ${quality.reason}`;
lastError = `quality check failed: ${quality.reason}`;

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.7.0 April 21, 2026 07:02
@diegosouzapw

Copy link
Copy Markdown
Owner

Subsumed by #1444 and manually merged

This was referenced Apr 30, 2026
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.

2 participants