feat(iorails): Add CompiledRail compatibility layer for rail actions - #2253
feat(iorails): Add CompiledRail compatibility layer for rail actions#2253tgasser-nv wants to merge 15 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThe PR introduces the manifest-driven
|
| Filename | Overview |
|---|---|
| nemoguardrails/guardrails/compiled_rail.py | Adds compilation, binding validation, dependency injection, execution, and per-rail model-call capture for manifest-backed actions. |
| nemoguardrails/guardrails/rail_guard.py | Centralizes redacted fail-closed outcomes and propagation of upstream HTTP-status exceptions. |
| nemoguardrails/guardrails/rail_action.py | Delegates existing rail exception handling to the shared guard. |
| nemoguardrails/guardrails/tool_rail_action.py | Adopts the shared error policy for synchronous tool checks. |
| nemoguardrails/llm/call.py | Records provider response identifiers for streaming and non-streaming model calls. |
| nemoguardrails/logging/explain.py | Adds a distinct provider request-ID field to LLM call records. |
| nemoguardrails/guardrails/iorails.py | Maps provider identifiers to the new request-ID field rather than the locally generated call identifier. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[Configured flow] --> Compile[compile_rail]
Compile --> Catalog[Manifest rail catalog]
Catalog --> Action[Resolved library action]
Action --> Execute[CompiledRail.execute]
Messages[Request messages] --> Dependencies[Request dependency mapping]
Dependencies --> Execute
Execute --> Guard[Shared rail error guard]
Execute --> Log[Isolated processing-log sink]
Guard --> Outcome[RailOutcome]
Log --> Calls[Captured LLMCallInfo records]
Reviews (6): Last reviewed commit: "Compact tests" | Re-trigger Greptile
|
@coderabbitai Review this PR |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis change adds manifest-driven ChangesRail execution and reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant CompiledRail
participant Action
participant LLMCallContext
Request->>CompiledRail: execute messages and request data
CompiledRail->>Action: invoke with declared dependencies and events
Action->>LLMCallContext: record LLM calls
CompiledRail-->>Request: return RailExecution
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
nemoguardrails/guardrails/rail_guard.py (1)
65-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA propagated failure is recorded twice on the same span.
Line 71 calls
record_span_error(span, exc). Line 77 re-raisesexc. All three call sites run insideaction_span, andaction_spancallsrecord_span_erroragain on any exception that escapes it (nemoguardrails/guardrails/telemetry.pylines 826-828). Every status-bearing failure therefore emits two exception events and setserror.typetwice on one span.Record the error only on the blocking path and let
action_spanown the propagating path.♻️ Proposed change
- record_span_error(span, exc) request_id = get_request_id() status = _upstream_http_status(exc) if status is not None: + # action_span records the error as the exception leaves the span. log.error("[%s] %s failed (HTTP %d): %s", request_id, action_name, status, exc) raise exc + record_span_error(span, exc) log.error("[%s] %s failed: %s", request_id, action_name, exc)
tests/guardrails/test_rail_guard.pylines 113-121 assert the recording directly on the helper with no enclosingaction_span, so that test needs updating to cover the span-level recording instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/guardrails/rail_guard.py` around lines 65 - 77, Update _blocked_reason_or_reraise to call record_span_error only when blocking the failure; for status-bearing exceptions, log and re-raise without recording so the enclosing action_span owns propagation-path recording. Adjust the direct helper test in test_rail_guard.py to verify recording through action_span rather than expecting _blocked_reason_or_reraise to record the error itself.nemoguardrails/guardrails/compiled_rail.py (2)
269-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail loudly on an unhandled binding kind.
_bind_parametershandlesliteralandsurface_param.contextis rejected earlier. Any otherBindingKindadded later falls through the loop and produces no binding, so the action silently runs with its default value. Raise instead, so a new kind surfaces as a compilation error.🛡️ Proposed fallback branch
if binding.kind == "surface_param": if key in params: bound.append(_BoundParameter(binding.action_param, params[key])) elif binding.required: raise RailCompilationError(f"{flow!r} is missing required parameter ${key}=") + continue + + raise RailCompilationError( + f"{flow!r} declares an unsupported binding kind {binding.kind!r} for {binding.action_param!r}" + ) # Context bindings are rejected before this point by # _reject_unfillable_binding_kinds, so there is nothing to freeze for them here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/guardrails/compiled_rail.py` around lines 269 - 290, Update _bind_parameters to add an explicit fallback after the literal, surface_param, and rejected-context handling that raises RailCompilationError for any unrecognized binding.kind. Include the flow, action parameter, and binding kind in the error, ensuring newly added BindingKind values cannot silently produce an unbound action parameter.
218-230: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate required
llmdependencies at compile time.
_request_dependencies()passesNonewhendeps.llmshas no"main". For actions with a requiredllmparameter, this defers a configuration error to request execution, whererail_error_outcomeconverts it to a block. Do not reject every action that listsllm:self_check_inputandself_check_outputdeclare it as optional and can use task-specific models fromllms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/guardrails/compiled_rail.py` around lines 218 - 230, Validate the required llm dependency during compilation before request execution, ensuring actions that require llm fail when self._deps.llms lacks a "main" model. Preserve compilation for self_check_input and self_check_output, whose llm parameter is optional and may resolve task-specific models from llms; keep _request_dependencies unchanged for valid configurations.tests/guardrails/test_rail_guard.py (1)
62-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for
rail_error_outcome.This file pins
rail_error_resultthoroughly.rail_guardalso exportsrail_error_outcome, which is only exercised indirectly throughcompile_railintests/guardrails/test_compiled_rail.py. A direct test pins theRailOutcomeshape and the propagation policy at the helper, so the coverage does not depend on compilation succeeding.💚 Proposed tests
+class TestOutcomeEnvelope: + """The RailOutcome variant applies the same policy as the RailResult variant.""" + + def test_unexpected_exception_returns_a_blocking_outcome(self): + """An arbitrary exception becomes a blocking RailOutcome with a redacted reason.""" + outcome = rail_error_outcome(None, ACTION_NAME, RuntimeError("auth rejected token nvapi-abc123secret")) + + assert outcome.is_blocked + assert outcome.reason == "content safety check input error: auth rejected token nvapi-***" + + `@status_bearing_types` + def test_exception_with_a_status_is_reraised(self, make_exc): + """A 503 propagates rather than becoming a block.""" + exc = make_exc(503) + + with pytest.raises(type(exc)) as excinfo: + rail_error_outcome(None, ACTION_NAME, exc) + + assert excinfo.value is excAdd the import:
-from nemoguardrails.guardrails.rail_guard import rail_error_result +from nemoguardrails.guardrails.rail_guard import rail_error_outcome, rail_error_result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/guardrails/test_rail_guard.py` around lines 62 - 99, Add direct tests for rail_error_outcome alongside the existing rail_error_result tests, importing the helper and RailOutcome as needed. Assert that non-status and status=None exceptions return the expected RailOutcome with is_safe=False and the sanitized reason, while exceptions carrying an HTTP status are reraised; verify the returned object’s shape directly without routing through compile_rail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoguardrails/guardrails/engine_registry.py`:
- Around line 178-179: Remove the added documentation text: delete the docstring
from _rollback_start in nemoguardrails/guardrails/engine_registry.py (lines
178-179), restore the prior test docstring text at
tests/guardrails/test_engine_registry.py lines 1143-1149, and remove the new
test docstrings at lines 1769-1774 and 1792-1796.
In `@nemoguardrails/guardrails/rail_guard.py`:
- Around line 74-80: Update the exception handling around _upstream_http_status
to redact str(exc) with _redact_secrets before either log.error call. Reuse the
redacted text in both the HTTP-status and non-status log paths, while preserving
the existing raise and client-facing return behavior.
In `@nemoguardrails/guardrails/tool_rail_action.py`:
- Around line 56-67: Update the _guarded method docstring to state that
rail_error_result propagates exceptions carrying an upstream HTTP status instead
of converting all errors into blocks, while retaining the fail-closed behavior
description for errors without an HTTP status.
In `@nemoguardrails/llm/call.py`:
- Line 103: Update _stream_llm_call to call _store_request_id with the
constructed response request ID before returning the LLMResponse, matching the
non-streaming path. Add a test covering a streamed response and verifying that
the provider request ID is stored.
In `@tests/guardrails/test_cross_engine_rail_equivalence.py`:
- Around line 244-248: Strengthen the assertions in the parity cases around the
llmrails and iorails response checks: for every blocked model or jailbreak case,
assert the response equals REFUSAL_MESSAGE, and for every allowed case, assert
it equals MAIN_OUTPUT. Replace the broad != MAIN_OUTPUT checks and extend the
existing content_safety_input_blocks-specific refusal assertion to cover all
blocked cases in both engine test sections.
---
Nitpick comments:
In `@nemoguardrails/guardrails/compiled_rail.py`:
- Around line 269-290: Update _bind_parameters to add an explicit fallback after
the literal, surface_param, and rejected-context handling that raises
RailCompilationError for any unrecognized binding.kind. Include the flow, action
parameter, and binding kind in the error, ensuring newly added BindingKind
values cannot silently produce an unbound action parameter.
- Around line 218-230: Validate the required llm dependency during compilation
before request execution, ensuring actions that require llm fail when
self._deps.llms lacks a "main" model. Preserve compilation for self_check_input
and self_check_output, whose llm parameter is optional and may resolve
task-specific models from llms; keep _request_dependencies unchanged for valid
configurations.
In `@nemoguardrails/guardrails/rail_guard.py`:
- Around line 65-77: Update _blocked_reason_or_reraise to call record_span_error
only when blocking the failure; for status-bearing exceptions, log and re-raise
without recording so the enclosing action_span owns propagation-path recording.
Adjust the direct helper test in test_rail_guard.py to verify recording through
action_span rather than expecting _blocked_reason_or_reraise to record the error
itself.
In `@tests/guardrails/test_rail_guard.py`:
- Around line 62-99: Add direct tests for rail_error_outcome alongside the
existing rail_error_result tests, importing the helper and RailOutcome as
needed. Assert that non-status and status=None exceptions return the expected
RailOutcome with is_safe=False and the sanitized reason, while exceptions
carrying an HTTP status are reraised; verify the returned object’s shape
directly without routing through compile_rail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 585c90d0-743d-477b-b454-76b5a72992f3
📒 Files selected for processing (18)
nemoguardrails/guardrails/compiled_rail.pynemoguardrails/guardrails/engine_registry.pynemoguardrails/guardrails/iorails.pynemoguardrails/guardrails/rail_action.pynemoguardrails/guardrails/rail_guard.pynemoguardrails/guardrails/tool_rail_action.pynemoguardrails/llm/call.pynemoguardrails/logging/explain.pynemoguardrails/logging/processing_log.pytests/guardrails/conftest.pytests/guardrails/test_compiled_rail.pytests/guardrails/test_cross_engine_rail_equivalence.pytests/guardrails/test_engine_registry.pytests/guardrails/test_iorails_generation_log_capture.pytests/guardrails/test_model_engine.pytests/guardrails/test_rail_guard.pytests/llm/test_call_import_graph.pytests/test_logging.py
Pouyanpi
left a comment
There was a problem hiding this comment.
Thanks @tgasser-nv, looks good. Let's ensure that we make CompiledRail’s limits explicit:
- reject unsupported callable types during compilation.
- ensure all required arguments can be supplied.
- reject surfaces needing unavailable context.
- continue supporting only async function actions for now.
- additional comments below.
| def _bind_parameters(surface: RailSurface, params: Mapping[str, str], flow: str) -> tuple[_BoundParameter, ...]: | ||
| """Freeze the manifest's bindings into concrete values, failing now if one cannot be.""" | ||
| bound: list[_BoundParameter] = [] | ||
| for binding in surface.bindings: | ||
| if binding.kind == "literal": | ||
| bound.append(_BoundParameter(binding.action_param, binding.value)) | ||
| continue | ||
|
|
||
| key = binding.key | ||
| if key is None: | ||
| raise RailCompilationError( | ||
| f"{flow!r} declares a {binding.kind} binding for {binding.action_param!r} with no source key" | ||
| ) | ||
|
|
||
| if binding.kind == "surface_param": | ||
| if key in params: | ||
| bound.append(_BoundParameter(binding.action_param, params[key])) | ||
| elif binding.required: | ||
| raise RailCompilationError(f"{flow!r} is missing required parameter ${key}=") | ||
| continue | ||
|
|
||
| # Context bindings are rejected by _reject_unfillable_binding_kinds before this | ||
| # point. Raise here for noisy visibility | ||
| raise RailCompilationError( | ||
| f"{flow!r} declares an unsupported {binding.kind!r} binding for {binding.action_param!r}" | ||
| ) | ||
| return tuple(bound) |
There was a problem hiding this comment.
could we reject parameters that aren’t declared by the surface? A typo like $varaint=custom is currently ignored, so the rail silently uses its default behavior.
| def _store_request_id(response: LLMResponse) -> None: | ||
| """Record the provider's response id on the current call, when it returned one. | ||
|
|
||
| Kept separate from ``LLMCallInfo.id``, which ``track_llm_call`` generates client-side: | ||
| only this value can be quoted to a provider, and only this value matches | ||
| ``gen_ai.response.id`` on the OTEL span for the same call, so overloading one field with | ||
| both meanings would make log-to-trace correlation unreliable. | ||
| """ | ||
| llm_call_info = llm_call_info_var.get() | ||
| if llm_call_info is None: | ||
| return | ||
| llm_call_info.request_id = response.request_id |
There was a problem hiding this comment.
could we also use this field in SpanExtractorV2? it currently is only checking raw_response["id"], so request IDs from normal and streaming calls don’t reach gen_ai.response.id. Or do you think this one is out of scope?
| "context": { | ||
| "user_message": _last_user_content(messages), | ||
| "bot_message": bot_response or "", | ||
| }, |
There was a problem hiding this comment.
could we reject surfaces whose context we can’t provide yet? self check hallucination compiles, but _last_bot_prompt is missing, so it returns allow without running the model.
could reproduce this: it returned allow with no model calls.
| record_span_error(span, exc) | ||
| log.error("[%s] %s failed: %s", request_id, action_name, detail) |
There was a problem hiding this comment.
I think you pointed this out recently. should we redact the exception before recording it on the span too? record_span_error exports the original exception message so things can get leaked.
|
|
||
| accepted = _accepted_parameters(action) | ||
| bound = _bind_parameters(surface, params, flow) | ||
| _reject_unaccepted_bindings(surface, action, bound, accepted, flow) |
There was a problem hiding this comment.
maybe we need to also check that every required action parameter can be filled? an unbound required parameter compiles and only becomes a TypeError and block on every request after the fact.
9952e53 to
a4dcba0
Compare
Description
This PR is the third in a series of stacked PRs to allow IORails to run the recently refactored actions in nemoguardrails/library/* directly and avoid duplicating actions for the two engines. The rough PR plan (this may change during implementation) is shown below. Note that PR3 in previous PRs has been split into two to keep the reviewable LOC for each manageable.
CompiledRailwhich is an IORails-specific wrapper around a rail from the shared Manifest rail system IORails and LLMRails share.The
CompiledRailclass is the executable unit behind one configured flow string, and is created dynamically from actions in thenemoguardrails/librarydirectory. Compiling the rail parses the flow and $params, pulls in theRailSurfacefrom the manifest, imports libraries for the rail, and freezes the plan to fill the action's parameters.The
CompiledRailclass isn't used at the moment, this will be implemented in PR3b (see stack above).Related Issue(s)
Verification
Unit-test
Integration test with Chat.
AI Assistance
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests