Skip to content

fix: map stream peek errors upstream - #1987

Merged
steebchen merged 1 commit into
mainfrom
inspect-finish-reason
Apr 7, 2026
Merged

steebchen merged 1 commit into
mainfrom
inspect-finish-reason

Conversation

@steebchen

@steebchen steebchen commented Apr 7, 2026 •

Copy link
Copy Markdown
Member

Summary

  • classify immediate streaming peek/read failures as upstream errors instead of the synthetic stream finish reason
  • preserve the existing stream_read_error code while mapping the loggable finish reason to upstream_error
  • add a regression test for a broken streaming body during immediate stream inspection

Verification

  • pnpm exec vitest run apps/gateway/src/chat/inspect-immediate-streaming-provider-error.spec.ts --no-file-parallelism
  • pnpm build
  • pnpm test:unit (fails on unrelated existing suites: apps/api/src/routes/beacon.spec.ts, apps/gateway/src/videos/videos.spec.ts)

Summary by CodeRabbit

  • Bug Fixes

    • Improved error reporting for streaming failures with accurate HTTP status codes (502) and descriptive error messages instead of generic placeholders.
  • Tests

    • Added test coverage for streaming error scenarios.

Copilot AI review requested due to automatic review settings April 7, 2026 18:38
@coderabbitai

coderabbitai Bot commented Apr 7, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

Made the inspectImmediateStreamingProviderError function exported and updated its error classification for stream read failures from "stream" to "upstream_error" with a 502 status code and "stream_read_error" status text. Added corresponding test coverage for the new error classification.

Changes

Cohort / File(s) Summary
Streaming error handling
apps/gateway/src/chat/chat.ts
Exported inspectImmediateStreamingProviderError function and modified immediate stream read error classification: errorType changed to "upstream_error", inferredStatusCode to 502, and statusText to "stream_read_error".
Test coverage
apps/gateway/src/chat/inspect-immediate-streaming-provider-error.spec.ts
New test file validating inspectImmediateStreamingProviderError behavior when a readable stream fails, asserting the returned error object matches the upstream_error structure with 502 status and "stream_read_error" code.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: map stream peek errors upstream' directly and clearly describes the main change: classifying immediate streaming errors as upstream errors rather than synthetic stream errors.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch inspect-finish-reason

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts how the gateway classifies immediate streaming peek/read failures so they’re treated as upstream/provider failures (rather than the synthetic stream finish reason), and adds a regression test to lock in the behavior.

Changes:

  • Reclassify immediate streaming read/peek exceptions as upstream_error with a synthetic 502 while preserving stream_read_error as the error code.
  • Export inspectImmediateStreamingProviderError for direct unit testing.
  • Add a Vitest regression test covering a broken streaming body during the immediate inspection phase.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
apps/gateway/src/chat/chat.ts Updates immediate streaming inspection error classification and exports the helper for testing.
apps/gateway/src/chat/inspect-immediate-streaming-provider-error.spec.ts Adds regression coverage for immediate stream read failures.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

async function inspectImmediateStreamingProviderError(
export async function inspectImmediateStreamingProviderError(

Copilot AI Apr 7, 2026

Copy link

Choose a reason for hiding this comment

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

inspectImmediateStreamingProviderError is now exported from chat.ts, which makes unit tests import the entire chat route module (large dependency graph + side effects like route/schema initialization). Consider moving this helper into apps/gateway/src/chat/tools/ (and testing that module directly), and keep chat.ts exporting only the route handler to reduce test overhead and coupling.

Suggested change
export async function inspectImmediateStreamingProviderError(
async function inspectImmediateStreamingProviderError(

Copilot uses AI. Check for mistakes.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
apps/gateway/src/chat/chat.ts (1)

592-592: Consider extracting this helper instead of exporting from chat.ts

Line 592 exports a route-internal helper mainly for test access. Prefer moving it to a small utility module and importing it from both chat.ts and the spec to keep this file’s public surface tighter and tests less coupled to route setup.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/gateway/src/chat/chat.ts` at line 592, The helper
inspectImmediateStreamingProviderError is being exported from chat.ts solely for
tests; extract it into a small utility module (e.g., create a new module
exporting inspectImmediateStreamingProviderError) and update chat.ts to import
and use that function internally instead of exporting it; then update the
test/spec to import the helper from the new utility module; ensure the original
function name inspectImmediateStreamingProviderError is preserved and exported
only from the new util to keep chat.ts’s public surface minimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/gateway/src/chat/chat.ts`:
- Line 592: The helper inspectImmediateStreamingProviderError is being exported
from chat.ts solely for tests; extract it into a small utility module (e.g.,
create a new module exporting inspectImmediateStreamingProviderError) and update
chat.ts to import and use that function internally instead of exporting it; then
update the test/spec to import the helper from the new utility module; ensure
the original function name inspectImmediateStreamingProviderError is preserved
and exported only from the new util to keep chat.ts’s public surface minimal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b67942cf-f2ea-46bb-b766-5155eadd790c

📥 Commits

Reviewing files that changed from the base of the PR and between 72569f6 and 1b62fb5.

📒 Files selected for processing (2)
  • apps/gateway/src/chat/chat.ts
  • apps/gateway/src/chat/inspect-immediate-streaming-provider-error.spec.ts

@steebchen
steebchen added this pull request to the merge queue Apr 7, 2026
Merged via the queue into main with commit 1a66808 Apr 7, 2026
21 of 22 checks passed
@steebchen
steebchen deleted the inspect-finish-reason branch April 7, 2026 18:55
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