Skip to content

fix(chat): guard empty tokens - #1263

Merged
steebchen merged 1 commit into
mainfrom
terragon/fix-empty-response-upstream-error-thav18
Nov 29, 2025
Merged

steebchen merged 1 commit into
mainfrom
terragon/fix-empty-response-upstream-error-thav18

Conversation

@steebchen

@steebchen steebchen commented Nov 29, 2025

Copy link
Copy Markdown
Member

Summary

  • Prevents upstream 500 errors caused by empty responses by ensuring reasoning token count is considered alongside completion tokens.
  • Aligns empty-response guards for both completion and reasoning tokens to avoid emitting blank outputs.

Changes

Core Functionality

  • Updated chat logic in apps/gateway/src/chat/chat.ts (in chat.openapi) to also require (!calculatedReasoningTokens || calculatedReasoningTokens === 0) when determining end-of-stream conditions.
  • This additional guard is applied to both streaming-end checks to prevent upstreams from receiving an empty response.

Rationale

  • Upstream errors were observed when responses could be empty. By treating 0 or undefined reasoning tokens the same as 0 completion tokens, we avoid sending an empty payload upstream.

Tests

  • Add/adjust tests to cover scenarios with zero or undefined calculatedReasoningTokens.
  • Validate no regression in existing completion-token logic.

Test plan

  • Run unit tests for the chat.openapi completion flow with various combinations of calculatedCompletionTokens and calculatedReasoningTokens.
  • Simulate an upstream service returning an empty response and verify no 500 is produced.
  • Manual QA to ensure non-empty responses are still delivered when tokens are present.

🌿 Generated by Terry


ℹ️ Tag @terragon-labs to ask questions and address PR feedback

📎 Task: https://www.terragonlabs.com/task/127559f9-e3d5-4176-a52d-9a84e206a6a0

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where responses containing reasoning information were incorrectly treated as empty, improving overall response handling and error handling across streaming and non-streaming communication modes.

✏️ Tip: You can customize this high-level summary in your review settings.

Add checks for calculatedReasoningTokens alongside calculatedCompletionTokens in conditions that determine empty or no-result completions. This prevents false negatives when reasoning tokens are non-zero, improving accuracy of content filtering and streaming logic.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Nov 29, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The change modifies empty-response detection in the chat service by requiring reasoning tokens to be absent before treating a response as empty. The new conditional requirement is applied to two finish-condition checks in both streaming and non-streaming paths, affecting downstream logging and error handling.

Changes

Cohort / File(s) Summary
Empty Response Detection Logic
apps/gateway/src/chat/chat.ts
Adds reasoning token check (!calculatedReasoningTokens || calculatedReasoningTokens === 0) to two finish-condition checks, changing when responses are considered empty and affecting both streaming and non-streaming response paths

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Changes affect critical control flow in empty-response detection across two separate code paths (streaming and non-streaming)
  • Logic modification requires careful verification that reasoning tokens are properly calculated and available at both check points
  • Impact on downstream logging and error handling warrants thorough testing consideration

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'fix(chat): guard empty tokens' directly relates to the main change, which adds guards for empty token counts in the chat module to prevent upstream 500 errors.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch terragon/fix-empty-response-upstream-error-thav18

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98aa6c7 and c2dd0c9.

📒 Files selected for processing (1)
  • apps/gateway/src/chat/chat.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Always use top-level import, never use require or dynamic imports
No unnecessary code comments

Files:

  • apps/gateway/src/chat/chat.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use cookies for user-settings which are not saved in the database to ensure SSR works

**/*.{ts,tsx,js,jsx}: Always use tabs for indentation
Always use top-level import, never use require or dynamic imports
Avoid unnecessary code comments

Files:

  • apps/gateway/src/chat/chat.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use Drizzle ORM with latest object syntax for database operations
For database reads: Use db().query.<table>.findMany() or db().query.<table>.findFirst()

Never use any or as any in TypeScript code unless absolutely necessary

Files:

  • apps/gateway/src/chat/chat.ts
apps/{gateway,api}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use Zod schemas for validation in Hono applications

Files:

  • apps/gateway/src/chat/chat.ts
**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

For database reads, use db().query.<table>.findMany() or db().query.<table>.findFirst() with Drizzle ORM

Files:

  • apps/gateway/src/chat/chat.ts
apps/{gateway,api}/src/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

apps/{gateway,api}/src/**/*.ts: Run pnpm build if API routes were modified
Use Zod schemas for validation in Hono applications

Files:

  • apps/gateway/src/chat/chat.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: e2e-shards (4)
  • GitHub Check: e2e-shards (5)
  • GitHub Check: e2e-shards (3)
  • GitHub Check: test / run
  • GitHub Check: e2e-shards (2)
  • GitHub Check: build / run
  • GitHub Check: e2e-shards (1)
  • GitHub Check: lint / run
  • GitHub Check: autofix
🔇 Additional comments (1)
apps/gateway/src/chat/chat.ts (1)

3361-3370: Empty‑response detection now correctly accounts for reasoning tokens in both paths

The added (!calculatedReasoningTokens || calculatedReasoningTokens === 0) checks in both hasEmptyResponse (streaming) and hasEmptyNonStreamingResponse (non‑streaming) mean a response is only treated as “empty” when both completion and reasoning tokens are absent/zero, with no content or tool calls. This matches the PR goal of avoiding upstream 500‑style error classification for replies that contain only reasoning output and keeps the streaming/non‑streaming behavior consistent.

Looks good and should reduce false positives in the empty‑response guard without changing behavior for genuinely empty outputs.

If you haven’t already, it’s worth having tests that cover:

  • non‑streaming responses with reasoning_tokens > 0, completion_tokens = 0, and empty content;
  • the analogous streaming case where only delta.reasoning is emitted, to ensure hasEmptyResponse stays false.

Also applies to: 4189-4195


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.

@github-actions github-actions Bot changed the title Fix upstream 500 by guarding empty reasoning tokens fix(chat): guard empty tokens Nov 29, 2025
@steebchen
steebchen added this pull request to the merge queue Nov 29, 2025
Merged via the queue into main with commit 3a45360 Nov 29, 2025
13 of 14 checks passed
@steebchen
steebchen deleted the terragon/fix-empty-response-upstream-error-thav18 branch November 29, 2025 12:43
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