review: pr#1384 #1571
review: pr#1384 #1571
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR attempts to clean up type annotations and simplify code in the guardrails integration, but introduces several logic bugs that will cause runtime errors and breaking behavior changes.
Major Issues Found
- Removed null safety check in
passthrough_fnthat will cause runtime errors whenpassthrough_inputis missing from context - Breaking change in
batch()method that lost per-input configuration support - all batched inputs now share the first config instead of individual configs - Added
return_exceptionsparameters to batch methods but completely unused, misleading callers about exception handling behavior - Removed safety checks in
_extract_text_from_input()causingKeyErrorfor missing dictionary keys and wrong string conversion forNonevalues - Changed
getattr()pattern for duck-typed objects that alters behavior when attributes exist but are falsy
Recommendations
This PR should not be merged as-is. The type annotation improvements in generation.py are good, but the changes in runnable_rails.py introduce too many functional regressions. Suggest reverting the functional changes and keeping only the type annotation updates.
Confidence Score: 1/5
- This PR is not safe to merge - contains multiple logic bugs that will cause runtime errors and breaking behavior changes
- Score reflects 8 critical logic issues found: removed null checks will cause runtime errors, batch() method breaks per-input configs (breaking change for existing users), unused return_exceptions parameters mislead callers, and removed safety checks cause KeyErrors. Only the generation.py changes are safe.
- Pay close attention to
nemoguardrails/integrations/langchain/runnable_rails.py- it has multiple critical bugs that need fixing before merge
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/actions/llm/generation.py | 5/5 | Updated copyright year to 2026 and added Tuple to type imports. Updated passthrough_fn type annotation to accept tuple returns. These are safe, well-aligned type annotation improvements. |
| nemoguardrails/integrations/langchain/runnable_rails.py | 2/5 | Updated copyright year, reorganized imports, removed type annotations, simplified multiple methods. Contains several issues: removed null check in passthrough_fn, broke per-input config handling in batch(), added unused return_exceptions parameter, removed safety checks in _extract_text_from_input. |
Sequence Diagram
sequenceDiagram
participant User
participant RunnableRails
participant LLMGenerationActions
participant Runnable
User->>RunnableRails: batch(inputs, config_list)
Note over RunnableRails: BUG: Uses config[0] for all inputs<br/>instead of individual configs
RunnableRails->>RunnableRails: invoke(input, config[0])
RunnableRails->>RunnableRails: _transform_input_to_rails_format()
alt Passthrough mode with runnable
RunnableRails->>RunnableRails: _create_passthrough_messages()
RunnableRails->>RunnableRails: _extract_text_from_input()
Note over RunnableRails: BUG: Can raise KeyError<br/>on missing dict key
RunnableRails->>LLMGenerationActions: passthrough_fn(context, events)
Note over LLMGenerationActions: BUG: _input can be None<br/>no null check
LLMGenerationActions->>Runnable: ainvoke(_input, config)
Runnable-->>LLMGenerationActions: output
LLMGenerationActions-->>RunnableRails: (text, output)
end
RunnableRails->>RunnableRails: rails.generate()
RunnableRails->>RunnableRails: _format_output()
RunnableRails-->>User: results
| async def passthrough_fn(context: dict, events: List[dict]) -> tuple[str, Any]: | ||
| async def passthrough_fn(context: dict, events: List[dict]): | ||
| # First, we fetch the input from the context | ||
| _input = context.get("passthrough_input") |
There was a problem hiding this comment.
Removed null check - context.get("passthrough_input") can return None, which will be passed directly to runnable.ainvoke(). This could cause runtime errors if the runnable doesn't handle None input gracefully.
Previous code checked if _input is None: raise ValueError(...) which provided clear error messaging.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 121:121
Comment:
Removed null check - `context.get("passthrough_input")` can return `None`, which will be passed directly to `runnable.ainvoke()`. This could cause runtime errors if the runnable doesn't handle `None` input gracefully.
Previous code checked `if _input is None: raise ValueError(...)` which provided clear error messaging.
How can I resolve this? If you propose a fix, please make it concise.| single_config = config[0] if isinstance(config, list) else config | ||
| return [self.invoke(input, single_config, **kwargs) for input in inputs] |
There was a problem hiding this comment.
Breaking change - lost per-input config support. Previous code handled config as a list by zipping with inputs: [self.invoke(inp, cfg, **kwargs) for inp, cfg in zip(inputs, config)]
Now always uses config[0] when config is a list, meaning all inputs use the same config. This breaks use cases where each input needs different configuration (e.g., different callbacks, metadata, or max_concurrency per input).
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 852:853
Comment:
Breaking change - lost per-input config support. Previous code handled `config` as a list by zipping with inputs: `[self.invoke(inp, cfg, **kwargs) for inp, cfg in zip(inputs, config)]`
Now always uses `config[0]` when config is a list, meaning all inputs use the same config. This breaks use cases where each input needs different configuration (e.g., different callbacks, metadata, or max_concurrency per input).
How can I resolve this? If you propose a fix, please make it concise.| inputs: List[Input], | ||
| config: Optional[Union[RunnableConfig, List[RunnableConfig]]] = None, | ||
| *, | ||
| return_exceptions: bool = False, |
There was a problem hiding this comment.
Added return_exceptions parameter but it's completely unused in the implementation. This is misleading - callers will expect exception handling behavior that doesn't exist.
Either implement the parameter (catch exceptions and return them in results when True) or remove it from the signature.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 848:848
Comment:
Added `return_exceptions` parameter but it's completely unused in the implementation. This is misleading - callers will expect exception handling behavior that doesn't exist.
Either implement the parameter (catch exceptions and return them in results when True) or remove it from the signature.
How can I resolve this? If you propose a fix, please make it concise.| inputs: List[Input], | ||
| config: Optional[Union[RunnableConfig, List[RunnableConfig]]] = None, | ||
| *, | ||
| return_exceptions: bool = False, |
There was a problem hiding this comment.
Added return_exceptions parameter but it's not used. The parameter is not passed to gather_with_concurrency, so exceptions will always propagate instead of being returned in the results when return_exceptions=True.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 860:860
Comment:
Added `return_exceptions` parameter but it's not used. The parameter is not passed to `gather_with_concurrency`, so exceptions will always propagate instead of being returned in the results when `return_exceptions=True`.
How can I resolve this? If you propose a fix, please make it concise.| return str(_input.content) | ||
| elif isinstance(_input, dict) and self.passthrough_user_input_key in _input: | ||
| value = _input.get(self.passthrough_user_input_key) | ||
| return str(value) if value is not None else "" | ||
| return str(_input[self.passthrough_user_input_key]) |
There was a problem hiding this comment.
Removed safety check - if _input.content is None or empty, str(None) returns "None" (the string). Previous code returned empty string for None/falsy content.
This changes behavior: str(None) != "". Could break logic expecting empty strings for missing content.
| return str(_input.content) | |
| elif isinstance(_input, dict) and self.passthrough_user_input_key in _input: | |
| value = _input.get(self.passthrough_user_input_key) | |
| return str(value) if value is not None else "" | |
| return str(_input[self.passthrough_user_input_key]) | |
| elif is_base_message(_input): | |
| return str(_input.content) if _input.content else "" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 202:204
Comment:
Removed safety check - if `_input.content` is `None` or empty, `str(None)` returns `"None"` (the string). Previous code returned empty string for `None`/falsy content.
This changes behavior: `str(None)` != `""`. Could break logic expecting empty strings for missing content.
```suggestion
elif is_base_message(_input):
return str(_input.content) if _input.content else ""
```
How can I resolve this? If you propose a fix, please make it concise.| elif isinstance(_input, dict) and self.passthrough_user_input_key in _input: | ||
| value = _input.get(self.passthrough_user_input_key) | ||
| return str(value) if value is not None else "" | ||
| return str(_input[self.passthrough_user_input_key]) |
There was a problem hiding this comment.
Direct dictionary access will raise KeyError if passthrough_user_input_key is not in _input. Previous code used .get() with fallback to handle missing keys safely.
| return str(_input[self.passthrough_user_input_key]) | |
| return str(_input.get(self.passthrough_user_input_key, "")) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 204:204
Comment:
Direct dictionary access will raise `KeyError` if `passthrough_user_input_key` is not in `_input`. Previous code used `.get()` with fallback to handle missing keys safely.
```suggestion
return str(_input.get(self.passthrough_user_input_key, ""))
```
How can I resolve this? If you propose a fix, please make it concise.| else: | ||
| # For duck-typed objects (including mocks in tests) | ||
| context = getattr(res, "output_data", None) or {} | ||
| context = getattr(res, "output_data", {}) or {} |
There was a problem hiding this comment.
Changed from getattr(res, "output_data", None) or {} to getattr(res, "output_data", {}) or {}.
The new code will return {} when output_data exists but is falsy (e.g., empty dict, None, 0, False). Previous code only used default {} when attribute didn't exist, preserving falsy but existing values. This changes behavior for duck-typed objects.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 648:648
Comment:
Changed from `getattr(res, "output_data", None) or {}` to `getattr(res, "output_data", {}) or {}`.
The new code will return `{}` when `output_data` exists but is falsy (e.g., empty dict, None, 0, False). Previous code only used default `{}` when attribute didn't exist, preserving falsy but existing values. This changes behavior for duck-typed objects.
How can I resolve this? If you propose a fix, please make it concise.| else: | ||
| # For duck-typed objects (including mocks in tests) | ||
| context = getattr(res, "output_data", None) or {} | ||
| context = getattr(res, "output_data", {}) or {} |
There was a problem hiding this comment.
Same issue as line 648 - behavior change for duck-typed objects. When output_data attribute exists but is falsy, this will return {} instead of preserving the falsy value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/runnable_rails.py
Line: 720:720
Comment:
Same issue as line 648 - behavior change for duck-typed objects. When `output_data` attribute exists but is falsy, this will return `{}` instead of preserving the falsy value.
How can I resolve this? If you propose a fix, please make it concise.
Pouyanpi
force-pushed
the
review/pr-1384
branch
from
bea2256 to
9cb0000
Compare
Pouyanpi
force-pushed
the
review/pr-1384
branch
from
9cb0000 to
4eaad2f
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Description
review #1384 to limit functional changes and fix pyright errors.