refactor(actions): resolve manifest actions lazily - #2186
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
b05380f to
33729cf
Compare
6231c7a to
92f2417
Compare
33729cf to
26ce747
Compare
92f2417 to
b373750
Compare
Greptile SummaryThis PR refactors the
|
| Filename | Overview |
|---|---|
| nemoguardrails/actions/action_dispatcher.py | Core change: adds lazy action registry (_lazy_action_refs, _manifest_action_names, _RegisteredActions view). Exception handling is correct at both get_action and execute_action; _manifest_action_names persists through resolution so is_manifest_action remains stable. Minor: values()/items() deliberately diverge from Mapping invariant (documented). |
| nemoguardrails/colang/v2_x/runtime/runtime.py | Adds is_rail_action flag to the _run_action result dict and _get_action_finished_event; is_manifest_action checked before _process_start_action so it fires correctly for both unresolved and already-cached manifest actions. |
| nemoguardrails/colang/runtime.py | Minor cleanup: action registration loop simplified, import_paths guard added, registered_actions return type updated to Mapping. |
| nemoguardrails/llm/filters.py | Replaces hardcoded system_actions name list with is_rail_action flag; removed names (self_check_, llama_guard_, alignscore_*, jailbreak_detection_heuristics, CallActivefenceApiAction, CallGcpnlpApiAction) are now covered dynamically via manifest ownership. |
| tests/test_action_dispatcher.py | Comprehensive new tests: lazy resolution and caching, override precedence, failure containment, log-injection escaping, and view semantics. Covers all new dispatcher code paths. |
| tests/rails/llm/test_builtin_rail_manifests.py | Adds test_every_manifested_action_is_declared_in_its_manifest: AST-walks all manifested library packages and asserts every @action function is declared in some manifest's refs, preventing silent omissions. |
| tests/test_injection_detection.py | Corrects caplog logger name from stale 'actions.py' to fully-qualified module name; fix was masked by eager loading before this PR. |
| tests/test_jailbreak_nim.py | Corrects two @patch targets from request module to actions module for jailbreak_nim_request; valid fix now that lazy loading makes the wrong target observable. |
| tests/v2_x/test_run_actions.py | Adds integration test confirming is_rail_action=True propagates from _run_action through _get_action_finished_event and is correctly filtered by co_v2. |
| tests/test_filters.py | Replaces name-based vendor action exclusion test with is_rail_action flag test, and adds positive case for non-rail custom actions. |
Sequence Diagram
sequenceDiagram
participant Catalog as RailCatalog
participant Dispatcher as ActionDispatcher
participant LazyRefs as _lazy_action_refs
participant ManifestNames as _manifest_action_names
participant RegActions as _registered_actions
participant Module as Action Module
Note over Dispatcher: __init__(load_all_actions=True)
Dispatcher->>Catalog: default_rail_catalog()
Dispatcher->>LazyRefs: _register_manifest_action_refs()
loop Each manifest ActionRef
LazyRefs-->>ManifestNames: add(action_ref.name)
end
Note over Module: Module NOT imported yet
Note over Dispatcher: First dispatch call
Dispatcher->>Dispatcher: execute_action(action_name)
Dispatcher->>Dispatcher: _normalize_action_name()
Dispatcher->>LazyRefs: _has_action_name() → True
Dispatcher->>Dispatcher: _resolve_registered_action()
Dispatcher->>Module: resolve_import_ref(action_ref)
Module-->>Dispatcher: resolved callable
Dispatcher->>RegActions: cache resolved action
Dispatcher->>LazyRefs: pop(action_name)
Note over ManifestNames: name retained → is_manifest_action() still True
Note over Dispatcher: Subsequent dispatch
Dispatcher->>RegActions: get directly (no import)
RegActions-->>Dispatcher: cached callable
Reviews (10): Last reviewed commit: "docs(actions): scope registered_actions ..." | Re-trigger Greptile
26ce747 to
cb7c03c
Compare
b373750 to
f959ca3
Compare
6a88f13 to
1e2ca85
Compare
f31d6a6 to
29e08df
Compare
1e2ca85 to
973f9f6
Compare
29e08df to
08d9c5c
Compare
📝 WalkthroughWalkthroughThe PR standardizes built-in rail action identifiers, adds lazy action resolution through rail manifests, updates moderation and blocked-response flows, generates a Rails configuration schema snapshot, and adds conformance and runtime equivalence tests. ChangesRail runtime and flow conformance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant ActionDispatcher
participant RailCatalog
participant ActionTarget
Runtime->>ActionDispatcher: initialize with rail_catalog
ActionDispatcher->>RailCatalog: register manifest action refs
Runtime->>ActionDispatcher: resolve action on use
ActionDispatcher->>ActionTarget: import referenced callable
ActionTarget-->>ActionDispatcher: return validated action
ActionDispatcher-->>Runtime: execute cached action
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/actions/action_dispatcher.py`:
- Around line 50-65: Update _RegisteredActions.__contains__ to check registered
action names through the dispatcher’s name registry, such as _action_names(),
without calling get_action or resolving imports. Ensure membership checks return
false for unknown names while preserving __getitem__ behavior for actual action
retrieval.
In `@schemas/rails_config.snapshot.json`:
- Around line 437-442: Update the owning action configuration field so its
advertised options match validation: either support sanitize in the field’s
pattern and implementation, or remove sanitize from the description. Then
regenerate schemas/rails_config.snapshot.json using the project’s
schema-generation tooling rather than editing the snapshot manually.
In `@tests/rails/llm/test_library_flow_files.py`:
- Around line 129-132: Update the declaration-only test’s action registration
loop to avoid calling resolve_import_ref(action_ref), keeping manifest actions
unresolved. Build or compare the dispatcher’s invoked names using the public
names indexed by each ActionRef, while preserving Python target resolution only
for execution-time paths.
- Around line 65-70: Validate the result from parse_colang_file before appending
it to parsed[version] in the file-parsing loop. Treat an empty result, including
the {} dialect-mismatch response, as a violation for the current manifest and
path, record an appropriate parse/mismatch message, and continue without
indexing it so later result["flows"] access cannot raise KeyError.
🪄 Autofix (Beta)
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: a3f06c26-4661-4378-9ebb-4962b978f77c
📒 Files selected for processing (33)
docs/configure-rails/guardrail-catalog/community/cleanlab.mdxdocs/configure-rails/guardrail-catalog/community/gcp-text-moderations.mdxnemoguardrails/actions/action_dispatcher.pynemoguardrails/colang/runtime.pynemoguardrails/library/activefence/flows.v1.conemoguardrails/library/autoalign/flows.conemoguardrails/library/cleanlab/actions.pynemoguardrails/library/cleanlab/flows.v1.conemoguardrails/library/cleanlab/rail.pynemoguardrails/library/context_bloat_detection/flows.v1.conemoguardrails/library/fiddler/actions.pynemoguardrails/library/fiddler/flows.v1.conemoguardrails/library/fiddler/rail.pynemoguardrails/library/gcp_moderate_text/actions.pynemoguardrails/library/gcp_moderate_text/flows.conemoguardrails/library/gcp_moderate_text/flows.v1.conemoguardrails/library/gcp_moderate_text/rail.pynemoguardrails/library/gliner/flows.v1.conemoguardrails/library/injection_detection/flows.conemoguardrails/library/polygraf/flows.v1.conemoguardrails/library/privateai/flows.v1.conemoguardrails/library/regex/flows.conemoguardrails/library/sensitive_data_detection/flows.v1.conemoguardrails/llm/filters.pyschemas/rails_config.snapshot.jsonscripts/generate_rails_config_schema_snapshot.pytests/rails/llm/test_builtin_rail_conformance.pytests/rails/llm/test_library_flow_files.pytests/test_action_dispatcher.pytests/test_filters.pytests/test_injection_detection.pytests/test_jailbreak_nim.pytests/test_runtime_flow_gate_equivalence.py
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
fe1b46c to
7e708b4
Compare
tgasser-nv
left a comment
There was a problem hiding this comment.
Looks good, please take a look at the comments before merging
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
3c69e70 to
2df7b0a
Compare
The registered_actions Mapping view resolved each name through get_action, so materializing it (values, items, dict(view)) imported every lazily declared action and raised KeyError on any whose optional dependency was missing. Override values and items to expose only already-resolved actions so bulk access never triggers lazy imports. Iteration, membership, and subscription keep their existing semantics: names cover the full declared namespace and subscripting still resolves a single action on demand.
Replace the full library tree walk and manifested-path skip set with a direct load of LEGACY_UNMANIFESTED_LIBRARY_PACKAGES. Manifested library actions are already indexed as lazy refs from the rail catalog, so the only packages that still need eager loading are the two pre-manifest ones. This drops the per-construction os.walk of the library and the re-import of every manifest origin module used only to compute directories to skip. A test pins the constant to the actual library layout so a new unmanifested action package fails loudly instead of silently never registering.
The dispatcher registers manifested library actions solely from their declared refs, so an `@action` in a manifested package that is missing from every manifest's refs would silently never register. Add the reverse of the existing ref-to-decorator check: scan each manifested library package for `@action` functions and assert each one appears in some manifest's action refs. The two legacy unmanifested packages are exempt.
Clarify that subscripting and dict(view) still resolve lazily (and can raise KeyError), while only values() and items() are guaranteed not to trigger imports. The prior wording implied any materialization was import-free.
Description
Resolve manifest-declared action references only when an action is dispatched.
This keeps catalog and configuration discovery independent of action-module
imports and optional rail dependencies while preserving explicitly registered
actions and existing dispatcher behavior.
Directly importing action symbols from
rail.pywould make discovery executethe runtime modules the manifest boundary is intended to isolate. structured
ActionRefvalues let the catalog index public action names first and resolvetheir Python targets only when execution reaches that action. Decorator-based
registration remains supported for user and custom actions; manifest-backed
resolution is the built-in library path, not its replacement.
teach both action dispatch and the Colang runtime to load the owning manifest
action on demand, cache the resulting registration through the existing action
registry, and retain clear unknown-action and import-failure behavior. Add
dispatcher coverage for successful lazy resolution, explicit registrations,
unknown actions, and failed imports.
two focused test fixes patch the action-bound injection logger and jailbreak
NIM request rather than stale definition-module symbols. Lazy import timing made
those incorrect patch targets observable; production behavior is unchanged.
default_rail_catalog)rail.pyrail.pyLLMRailsinit with one rail enabledImpact
built-in action module or its optional runtime dependencies.
dispatched, after which the existing registry caches it.
the execution boundary with focused test coverage.
and dispatcher semantics remain unchanged.
Stack
pouyanpi/rail-library-stack-3-manifest-contractdeveloppouyanpi/rail-library-stack-4-builtinspouyanpi/rail-library-stack-4b-flow-gatepouyanpi/rail-library-stack-5-lazy-actionsVerification
AI Assistance
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests