Skip to content

fix(server): emit terminal SSE frames for rail execution failures - #2513

Merged
Pouyanpi merged 2 commits into
NVIDIA-NeMo:developfrom
Pouyanpi:pouyanpi/qa-bug-stream-rail-error-frame
Oct 29, 2026
Merged

Pouyanpi merged 2 commits into
NVIDIA-NeMo:developfrom
Pouyanpi:pouyanpi/qa-bug-stream-rail-error-frame

Conversation

@Pouyanpi

@Pouyanpi Pouyanpi commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Recognize internal_error stream chunks as terminal errors so rail execution failures emit an SSE error frame followed by [DONE], instead of appearing in delta.content.

Add regression coverage for string/dict chunks, stream termination, and sequential/parallel rail failures through the HTTP endpoint, including IORails fallback.

Summary by CodeRabbit

  • Bug Fixes
    • Streaming responses now report rail execution failures as structured errors and stop sending subsequent content. This applies to sequential and parallel output rails, including legacy and IORails fallback engines.
    • Streaming error metadata can now identify internal errors.

Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
@github-actions github-actions Bot added status: needs triage New issues that have not yet been reviewed or categorized. size: S labels Oct 28, 2026
@Pouyanpi Pouyanpi self-assigned this Oct 28, 2026
@Pouyanpi Pouyanpi added status: triaged Triaged by a maintainer; eligible for automated review (CodeRabbit/Greptile). and removed status: needs triage New issues that have not yet been reviewed or categorized. labels Oct 28, 2026
@greptile-apps

greptile-apps Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable defects were found.

Summary

Rail execution failures now produce a terminal SSE error frame instead of appearing in delta.content.

  • Streaming chat replies now stop on rail execution failures.

No actionable issues found. Tests were inspected but not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Rail action fails] --> B[internal_error chunk]
  B --> C[Shared error-type check]
  C --> D[SSE error frame]
  D --> E[DONE]
  E --> F[No later content]
Loading

Reviews (2) · Last reviewed commit: "fix(streaming): share terminal error typ..." · Reviewed by Greptile

@coderabbitai

coderabbitai Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between e6b1c2b and 4315557.

Files selected for processing: 2
  • nemoguardrails/server/api.py
  • tests/server/test_api.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Walkthrough

The API accepts internal_error as a chunk error type. New streaming tests cover error-frame termination and output-rail failures across engine variants and rail execution modes.

Changes: Streaming errors

Layer / File(s) Summary
Error chunk typing and stream tests
nemoguardrails/server/api.py, tests/server/test_api.py
ChunkErrorMetadata.type now accepts internal_error. Streaming tests check that error chunks and output-rail failures produce an error frame followed by [DONE], without later unchecked content.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

Possibly related PRs

  • NVIDIA-NeMo/Guardrails#2278: Adds server handling for terminal ChunkError frames, which this change extends to accept internal_error.

Suggested labels: bug

Suggested reviewers: m-misiura, tgasser-nv

Merge Risk: Minimal · up to 43155

Internal errors are intended to terminate streams with an error frame. No merge-blocking issue is established.

Pre-merge checks: Passed 5 · Failed 1

Failed 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%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 5 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "fix(server): emit terminal SSE frames for rail execution failures" matches the main change. The summary and objectives say the change treats internal_error stream chunks as terminal errors,…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Test Results For Major Changes Passed The PR changes terminal SSE error behavior and adds regression tests for string/dict chunks, stream termination, output-rail failures, invalid outcomes, sequential/parallel rails, and IORails fallback…
Full details: Title check

Explanation

The title "fix(server): emit terminal SSE frames for rail execution failures" matches the main change. The summary and objectives say the change treats internal_error stream chunks as terminal errors, so rail execution failures emit an SSE error frame followed by [DONE] instead of appearing in delta.content. The title is specific and uses a conventional commit prefix. It does not mention the ChunkErrorMetadata type change, but that is a supporting detail.

Full details: Test Results For Major Changes

Explanation

The PR changes terminal SSE error behavior and adds regression tests for string/dict chunks, stream termination, output-rail failures, invalid outcomes, sequential/parallel rails, and IORails fallback. The PR description documents this testing coverage. No numeric, convergence, or performance change is indicated.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@tgasser-nv tgasser-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Just a couple of small items to address before merging. Thanks for the quick-fix.

Comment thread nemoguardrails/server/api.py Resolved
Comment thread tests/server/test_api.py Resolved
…lures

Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
@github-actions github-actions Bot added size: M and removed size: S labels Oct 29, 2026
@Pouyanpi
Pouyanpi merged commit 6828f81 into NVIDIA-NeMo:develop Oct 29, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: M status: triaged Triaged by a maintainer; eligible for automated review (CodeRabbit/Greptile).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants