Skip to content

Conversation

@Pouyanpi
Copy link
Collaborator

@Pouyanpi Pouyanpi commented Nov 24, 2025

PR Description

Fixes issue #1505 where PR #1380 introduced a TypeError in the v2.x chat CLI due to incorrect type conversion between State and dict.

Root Cause:

  • The type annotation for LLMRails.process_events_async() was incorrect (claimed to return dict for both v1.0 and v2.x)
  • v2.x runtime actually returns State objects, not dicts
  • PR chore(types): Type-clean /cli (37 errors) #1380 "fixed" chat.py based on the wrong annotation, adding asdict() and State(**output_state) conversions
  • This caused: TypeError: State() argument after ** must be a mapping, not State

Why Tests Didn't Catch This:

  • All existing v2.x tests called runtime.process_events() directly, bypassing the LLMRails wrapper
  • CLI tests only used mocks, never executing actual code
  • No integration tests existed for LLMRails.process_events_async() with v2.x

Running

nemoguardrails chat --config=./examples/configs/nemoguards_v2

works as before

…ersion

Fixes incorrect type assumptions in PR #1380 that caused TypeError when
using v2.x chat CLI. The bug incorrectly assumed process_events_async
returns dict and tried to convert State objects with
State(**output_state).

Changes:
- Fix type annotations in LLMRails.process_events_async to return
Union[dict, State]
- Remove incorrect asdict() conversion and State(**) reconstruction in
chat.py
- Add integration tests to prevent regression
@Pouyanpi Pouyanpi added this to the v0.19.0 milestone Nov 24, 2025
@Pouyanpi Pouyanpi self-assigned this Nov 24, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 24, 2025

Greptile Overview

Greptile Summary

This PR correctly fixes a TypeError introduced in PR #1380 where the v2.x chat CLI would crash when processing events.

Root Cause Analysis:

  • PR chore(types): Type-clean /cli (37 errors) #1380 incorrectly assumed LLMRails.process_events_async() returns a dict for the state parameter
  • The type annotation in llmrails.py:1468 was wrong, claiming to return dict for both v1.0 and v2.x
  • In reality, v2.x runtime's RuntimeV2_x.process_events() returns a State object (as shown in runtime.py:403)
  • PR chore(types): Type-clean /cli (37 errors) #1380 added conversions: asdict(chat_state.state) before calling the method and State(**output_state) after
  • This caused: TypeError: State() argument after ** must be a mapping, not State

The Fix:

  1. chat.py: Removed incorrect conversions - now passes State objects directly to process_events_async() and uses simple cast(State, output_state) instead of State(**output_state)
  2. llmrails.py: Fixed type annotations to accurately reflect Union[Optional[dict], State] for both input and output
  3. Tests: Added integration tests that call LLMRails.process_events_async() directly (previous tests only called runtime.process_events(), bypassing the wrapper)

Key Changes:

  • Removed asdict import from chat.py (no longer needed)
  • Changed lines 501, 506, 537, and 541 in chat.py to handle State objects correctly
  • Updated type signatures in llmrails.py at lines 1466, 1468, 1505, and 1507
  • Added 253 lines of comprehensive integration tests gated behind LIVE_TEST_MODE

The fix is minimal, targeted, and addresses the exact root cause without over-engineering.

Confidence Score: 5/5

  • This PR is safe to merge - it's a targeted bug fix that corrects a TypeError that broke the v2.x chat CLI
  • Score of 5 (safe to merge) because: (1) The fix directly addresses the root cause by correcting both the type annotations and the incorrect conversions, (2) The changes are minimal and focused - only removing problematic code, not adding complexity, (3) Comprehensive integration tests were added that would have caught this bug, (4) The PR author manually tested the fix with the actual CLI command, (5) No other code uses the removed asdict/State(**) pattern, and (6) The fix aligns the code with the actual runtime behavior that has existed all along
  • No files require special attention - all changes are straightforward bug fixes

Important Files Changed

File Analysis

Filename Score Overview
nemoguardrails/cli/chat.py 5/5 Removes incorrect State/dict conversions (asdict and State(**)) at lines 501 and 506, 537 and 541. Now correctly passes State objects directly to process_events_async. Also removes unused asdict import.
nemoguardrails/rails/llm/llmrails.py 5/5 Corrects type annotations for process_events_async and process_events methods to accurately reflect that they accept and return Union[dict, State] instead of just dict, fixing the root cause of the type confusion.
tests/cli/test_chat_v2x_integration.py 5/5 Adds comprehensive integration tests that would have caught the bug. Tests verify LLMRails.process_events_async returns State objects and accepts State objects as input. Tests are gated behind LIVE_TEST_MODE to avoid unnecessary API calls.

Sequence Diagram

sequenceDiagram
    participant User
    participant ChatCLI as chat.py (_run_chat_v2_x)
    participant LLMRails as llmrails.py (LLMRails)
    participant RuntimeV2 as runtime.py (RuntimeV2_x)
    
    Note over User,RuntimeV2: Before Fix (PR #1380 - BROKEN)
    User->>ChatCLI: User input
    ChatCLI->>ChatCLI: asdict(chat_state.state)
    Note right of ChatCLI: Incorrectly converts State to dict
    ChatCLI->>LLMRails: process_events_async(events, dict_state)
    LLMRails->>RuntimeV2: process_events(events, dict_state)
    RuntimeV2-->>LLMRails: (output_events, State)
    Note right of RuntimeV2: Returns State object
    LLMRails-->>ChatCLI: (output_events, State)
    ChatCLI->>ChatCLI: State(**output_state)
    Note right of ChatCLI: TypeError! output_state is already State
    ChatCLI--xUser: CRASH
    
    Note over User,RuntimeV2: After Fix (This PR - CORRECT)
    User->>ChatCLI: User input
    ChatCLI->>LLMRails: process_events_async(events, State)
    Note right of ChatCLI: Pass State object directly
    LLMRails->>RuntimeV2: process_events(events, State)
    RuntimeV2-->>LLMRails: (output_events, State)
    Note right of RuntimeV2: Returns State object
    LLMRails-->>ChatCLI: (output_events, State)
    ChatCLI->>ChatCLI: cast(State, output_state)
    Note right of ChatCLI: Simple type cast, no conversion
    ChatCLI-->>User: Success!
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@Pouyanpi Pouyanpi requested a review from cparisien November 24, 2025 13:18
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@NVIDIA-NeMo NVIDIA-NeMo deleted a comment from codecov bot Nov 24, 2025
@Pouyanpi Pouyanpi merged commit c34f294 into develop Nov 24, 2025
8 checks passed
@Pouyanpi Pouyanpi deleted the fix/cli-v2x-state-conversion-issue-1505 branch November 24, 2025 16:16
@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nemoguardrails/cli/chat.py 33.33% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Pouyanpi added a commit that referenced this pull request Nov 26, 2025
#1509)

* fix(cli): Fix TypeError in v2.x chat due to incorrect State/dict conversion

Fixes incorrect type assumptions in PR #1380 that caused TypeError when
using v2.x chat CLI. The bug incorrectly assumed process_events_async
returns dict and tried to convert State objects with
State(**output_state).

Changes:
- Fix type annotations in LLMRails.process_events_async to return
Union[dict, State]
- Remove incorrect asdict() conversion and State(**) reconstruction in
chat.py
- Add integration tests to prevent regression
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.

3 participants