-
Notifications
You must be signed in to change notification settings - Fork 551
fix(runtime): ensure stop flag is set for policy violations in parallel rails #1467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR fixes a critical bug in the parallel rails execution path where policy violations (stopped rails) were not properly recorded in the processing log. The fix ensures that when a rail detects a policy violation and stops, its processing log is now included in the aggregated result and the OutputRailFinished/InputRailFinished post-event is correctly omitted. This allows compute_generation_log() to create the required ActivatedRail entry with stop=True, enabling client code to identify which specific guardrail triggered and why.
How it works:
- In
runtime.py, when a parallel rail stops (detected via a "stop"BotIntentevent), the code now captures that rail's processing log before canceling the task - The stopped rail's processing log is filtered (removing
Listenandstart_flowevents) and merged into the main processing log - Post-events are now conditionally appended only when no stop event is present, preventing invalid event sequences
- Five new test cases verify that stopped rails appear in
activated_railswithstop=True, exactly one rail is marked as stopped per violation, and stopped/non-stopped rails are mutually exclusive
Potential Issues:
- Lines 446-453: The filtering logic that removes
Listenandstart_flowevents from stopped task logs may be too aggressive—if these events contain critical context about why the rail stopped, discarding them could make debugging harder - Lines 315-324: The stop detection relies on string matching
event["intent"] == "stop"without validating the event structure; if the event dictionary is malformed (missing "intent" key), this will raise aKeyError - Test coverage gap: The new tests only validate the Colang 1.0 runtime path but the fix is isolated to
v1_0/runtime/runtime.py; if parallel rails exist in v2_0, they may still have the bug - Line 389: The
stopped_task_processing_logis captured after the task is canceled—there's a potential race condition where the processing log could be incomplete if the cancellation happens before all events are recorded
Confidence: 3/5 — The fix correctly addresses the stated problem and test coverage is good, but the event filtering logic and lack of error handling for malformed events introduce risk. Recommend adding validation for the event structure and clarifying whether Listen/start_flow events should really be discarded from stopped rails.
2 files reviewed, 1 comment
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where policy violations in parallel rails were not properly tracked. Specifically, when a rail stopped due to a policy violation, its processing log was excluded from aggregation, preventing the creation of an ActivatedRail entry for the stopped rail. Additionally, the OutputRailFinished/InputRailFinished events were incorrectly added even when a rail stopped.
Key Changes:
- Modified the
task_call_helperto check for stop events and conditionally add post_events - Added tracking of stopped rail processing logs to include them in the final aggregation
- Added comprehensive test coverage for parallel rail stop behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_parallel_rails.py |
Adds four new test cases to verify stop flag behavior for input rails, output rails, client code patterns, and multiple activated rails |
nemoguardrails/colang/v1_0/runtime/runtime.py |
Implements stop event detection, conditional post_event addition, and stopped rail processing log aggregation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
19a574f to
c3594ae
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Pouyan <13303554+Pouyanpi@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR fixes a critical bug in the parallel rails execution path within the Colang v1.0 runtime. When multiple guardrails ran concurrently and one violated a policy (stopped execution), the stopped rail's processing log was incorrectly excluded from aggregation, preventing compute_generation_log() from creating an ActivatedRail entry for the stopped rail. Additionally, post-events (InputRailFinished/OutputRailFinished) were incorrectly emitted even when a rail stopped mid-execution. The fix introduces a stopped_task_processing_log variable to capture logs from stopped tasks, refactors log filtering into a reusable filter_and_append helper function, and conditionally appends post-events only when no stop flag is detected. This ensures proper tracking of policy violations in parallel rail scenarios and accurate event sequencing in the Guardrails runtime.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/colang/v1_0/runtime/runtime.py | 4/5 | Refactored parallel rail execution to capture stopped task logs via stopped_task_processing_log, added filter_and_append helper to unify log filtering logic, and conditionally emits post-events only when no stop flag is present |
| tests/test_parallel_rails.py | 5/5 | Added four test cases verifying stop flag correctness on ActivatedRail entries for both input and output rails in parallel execution, including a client code pattern test for identifying which specific rail stopped |
Confidence score: 4/5
- This PR addresses a well-defined bug with a focused fix; the changes are localized to the parallel rails execution path and backed by comprehensive test coverage.
- Score reflects the complexity of the async task orchestration logic in
runtime.pyand the need to ensure stopped task logs are correctly captured in all edge cases (e.g., multiple rails stopping simultaneously, race conditions in task completion). - Pay close attention to
nemoguardrails/colang/v1_0/runtime/runtime.pylines 315-324 and 447-458, where the conditional post-event logic and log filtering are implemented; verify that thehas_stopcheck correctly identifies stop events and thatfilter_and_appendexcludes the correct event types.
Sequence Diagram
sequenceDiagram
participant User
participant LLMRails
participant Runtime as RuntimeV1_0
participant TaskManager as Async Task Manager
participant Rail1 as Rail Flow 1
participant Rail2 as Rail Flow 2
participant RailN as Rail Flow N
User->>LLMRails: generate_async(messages)
LLMRails->>Runtime: _run_flows_in_parallel(flows, events)
Note over Runtime,TaskManager: Create tasks for each rail flow
Runtime->>TaskManager: create_task(Rail1 + pre_event)
Runtime->>TaskManager: create_task(Rail2 + pre_event)
Runtime->>TaskManager: create_task(RailN + pre_event)
par Parallel Execution
TaskManager->>Rail1: generate_events(events, processing_log)
TaskManager->>Rail2: generate_events(events, processing_log)
TaskManager->>RailN: generate_events(events, processing_log)
end
Note over Rail1,RailN: Process flows in parallel
alt Rail2 has stop event (BotIntent with intent="stop")
Rail2-->>TaskManager: return result with stop event
TaskManager->>Runtime: flow_uid, result (has_stop=True)
Runtime->>TaskManager: Cancel all pending tasks
TaskManager->>Rail1: cancel()
TaskManager->>RailN: cancel()
Note over Runtime: Store stopped_task_results and stopped_task_processing_log
Note over Runtime: Exclude post_event for stopped rail
else All rails complete successfully
Rail1-->>TaskManager: return result (no stop)
Rail2-->>TaskManager: return result (no stop)
RailN-->>TaskManager: return result (no stop)
TaskManager->>Runtime: Collect results in flow order
Note over Runtime: Add post_events for completed rails
end
Note over Runtime: Aggregate processing logs
Runtime->>Runtime: Filter out "Listen" and "start_flow" events
Runtime->>Runtime: Append stopped rail logs first
Runtime->>Runtime: Append finished rails logs
Runtime->>Runtime: Create EventHistoryUpdate with finished_task_results
Runtime->>Runtime: Create ActionResult with stopped_task_results
Runtime-->>LLMRails: ActionResult(events, context_updates)
LLMRails->>LLMRails: compute_generation_log()
Note over LLMRails: Create ActivatedRail entries with stop flag
LLMRails-->>User: GenerationResponse with log.activated_rails
2 files reviewed, 1 comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This review covers only the latest changes since the previous review. The most recent commit addresses the variable naming inconsistency by renaming stopped_task_processing_log to stopped_task_processing_logs (plural), matching the convention used for similar list variables in the function. This fix is critical because the previous singular name created a new local variable instead of properly populating the collection that gets aggregated into the main processing_log at line 457. The consequence was that stopped rails were invisible to compute_generation_log(), which failed to create ActivatedRail entries for policy violations. By correcting the name, the stopped rail's processing log is now properly merged via filter_and_append(), ensuring guardrails reporting accurately reflects which rails stopped and why.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/colang/v1_0/runtime/runtime.py | 5/5 | Fixed variable name from singular to plural, ensuring stopped rail processing logs are properly aggregated for downstream reporting |
Confidence score: 5/5
- This PR is safe to merge with minimal risk
- The change is a straightforward variable rename that fixes a critical bug where stopped rails were not being reported; the fix ensures previously captured data is now properly included in aggregation, with no new logic paths or edge cases introduced
- No files require special attention
1 file reviewed, no comments
When rails ran in parallel, the stopped rail's processing log was excluded from aggregation, causing
compute_generation_log()to never create anActivatedRailentry for the stopped rail. Additionally,the
post_event(OutputRailFinished/InputRailFinished) was incorrectly added even when a rail stopped.