chore(iorails): Refactor RailsManager and Nemoguard Actions#1762
Open
tgasser-nv wants to merge 5 commits intodevelopfrom
Open
chore(iorails): Refactor RailsManager and Nemoguard Actions#1762tgasser-nv wants to merge 5 commits intodevelopfrom
tgasser-nv wants to merge 5 commits intodevelopfrom
Conversation
Contributor
Greptile SummaryThis PR refactors the IORails engine from a monolithic
The two previously flagged P0/P1 issues ( The remaining findings are minor:
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/rail_action.py | New base class introducing the template-method pipeline (validate → extract → prompt → respond → parse); fail-safe exception handling wraps both _get_response and _parse_response; no issues found. |
| nemoguardrails/guardrails/rails_manager.py | Refactored from a monolithic impl to delegating to RailAction instances; _ACTION_CLASSES registry and _create_action cleanly replace the old if/elif dispatch; sequential and parallel execution logic unchanged. |
| nemoguardrails/library/content_safety/iorails_actions.py | Clean extraction of content safety logic into ContentSafetyInputAction and ContentSafetyOutputAction; _content_safety_to_rail_result correctly handles [False] single-element case; minor redundancy: _require_model_type called 3× per request. |
| nemoguardrails/library/jailbreak_detection/iorails_actions.py | Clean and concise; correctly uses _get_api_response without requiring a model type; parse logic handles missing 'jailbreak' field with a RuntimeError that is caught fail-safe in the base run(). |
| nemoguardrails/library/topic_safety/iorails_actions.py | Topic safety action correctly renders a static system prompt (no user variables) and passes full message history separately; _parse_response calls .lower().strip() on an Any-typed response, but this is within the try/except in the base run() and is safe. |
| tests/guardrails/test_rail_action.py | Good coverage of the base class pipeline, helpers, and error paths; DummyRailAction does not declare action_name, requiring tests that exercise _validate_flow_name to set it manually — fragile for future additions. |
| tests/guardrails/test_rails_manager.py | Updated to use the new action-delegation architecture; removed tests of now-deleted private helpers; retained integration tests for sequential/parallel execution and short-circuit behavior. |
Sequence Diagram
sequenceDiagram
participant Caller
participant RailsManager
participant RailAction
participant ModelManager
Caller->>RailsManager: is_input_safe(messages)
RailsManager->>RailsManager: build rails dict {flow: _run_rail(flow,...)}
alt Sequential
loop each flow
RailsManager->>RailAction: run(flow, messages, bot_response)
RailAction->>RailAction: _validate_input()
RailAction->>RailAction: _extract_messages()
RailAction->>RailAction: _create_prompt()
RailAction->>ModelManager: _get_response() [LLM or API]
ModelManager-->>RailAction: raw response
RailAction->>RailAction: _parse_response()
RailAction-->>RailsManager: RailResult
alt unsafe
RailsManager-->>Caller: RailResult(is_safe=False) [short-circuit]
end
end
RailsManager-->>Caller: RailResult(is_safe=True)
else Parallel
par all flows
RailsManager->>RailAction: run(flow, messages, bot_response)
RailAction-->>RailsManager: RailResult
end
RailsManager-->>Caller: first unsafe RailResult or RailResult(is_safe=True)
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/guardrails/test_rail_action.py
Line: 1311-1317
Comment:
**`DummyRailAction` missing `action_name` class attribute**
`RailAction` declares `action_name: str` as a required class-level attribute. `DummyRailAction` never defines it, so any test that reaches `_validate_flow_name` without first manually setting `dummy_action.action_name = ...` will get a confusing `AttributeError` rather than the documented `RuntimeError`.
The current test suite works only because `test_mismatched_flow_name_raises` explicitly sets the attribute before calling `_validate_flow_name`. A future test that forgets this step will produce a misleading failure.
Consider adding the attribute at the class level:
```suggestion
class DummyRailAction(RailAction):
"""Minimal concrete subclass that records calls for testing."""
action_name = "dummy rail"
def __init__(self, *args, **kwargs):
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: nemoguardrails/library/content_safety/iorails_actions.py
Line: 53-54
Comment:
**`model_type` parameter is shadowed immediately**
`_get_response` is declared with a `model_type: Optional[str]` parameter (coming from the base class `run()` call on line 84 of `rail_action.py`), but the very first line discards it and re-derives the value via `_require_model_type(flow)`. This pattern appears identically in `ContentSafetyOutputAction._get_response` (line 108) and `TopicSafetyInputAction._get_response`.
Because `_validate_input` already confirmed `$model=` is present, the re-call is always safe, but it makes the parameter misleading: readers of the abstract interface expect the passed value to be used.
Consider either using the parameter directly, or removing it from the abstract signature if subclasses are expected to re-derive it themselves:
```suggestion
async def _get_response(self, flow: str, prompt: Any, model_type: Optional[str]) -> str:
if model_type is None:
model_type = self._require_model_type(flow)
task_key = f"content_safety_check_input $model={model_type}"
```
The same applies to `ContentSafetyOutputAction._get_response` and `TopicSafetyInputAction._get_response`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "Update RailsManager to delegate to Rails..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@greptile review latest commit and update score/summary |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR improves the decoupling between RailsManager and the rails it runs. The changes are:
run()method, mandatory methods for subclasses to define, and shared helpers that apply to all rails.iorails_actions.pyfile in thenemoguardrails/library/<action_name>directory. This combines the Python and Colang files into a single base-class with a defined interface.Related Issue(s)
Test Plan
Pre-commit
Note The
nemoguardrails/librarydirectory isn't covered by Pyright automatic checking yet, this is blocked on #1389. I checked locally and it's clean.Unit-test
Chat application
Server API
Client
Server
Checklist