fix: apply review suggestions by mike

Author: Pouyanpi

nemoguardrails/actions/actions.py

@@ -37,8 +37,9 @@ def action(
         name (Optional[str]): The name to associate with the action.
         execute_async: Whether the function should be executed in async mode.
         output_mapping (Optional[Callable[[Any], bool]]): A function to interpret the action's result.
-            It should accept the return value (e.g. the first element of a tuple) and return True if the output
-            should be considered blocked.
+            It accepts the return value (e.g. the first element of a tuple) and return True if the output
+            is not safe.
+
     Returns:
         callable: The decorated function or class.
     """

nemoguardrails/actions/llm/generation.py

@@ -766,7 +766,7 @@ async def generate_bot_message(
         # we must disable (skip) the output rails which gets executed on $bot_message
         # as it is executed separately in llmrails.py
         # of course, it does not work when passed as context in `run_output_rails_in_streaming`
-        # streming_handler is set when stream_async method is used
+        # streaming_handler is set when stream_async method is used
         if streaming_handler and self.config.rails.output.streaming.enabled:
             context_updates["skip_output_rails"] = True
 
@@ -787,9 +787,7 @@ async def generate_bot_message(
             context_updates["skip_output_rails"] = True
 
         # Check if the output is supposed to be the content of a context variable
-        # TODO: add this bug fix
-        # elif bot_intent and bot_intent[0] == "$" and bot_intent[1:] in context:
-        elif bot_intent[0] == "$" and bot_intent[1:] in context:
+        elif bot_intent and bot_intent[0] == "$" and bot_intent[1:] in context:
             bot_utterance = context[bot_intent[1:]]
 
         else:

nemoguardrails/actions/output_mapping.py

@@ -31,8 +31,8 @@ def default_output_mapping(result: Any) -> bool:
         return False
 
 
-def should_block_output(result: Any, action_func: Any) -> bool:
-    """Determines if an action result should be blocked using its attached mapping.
+def is_output_blocked(result: Any, action_func: Any) -> bool:
+    """Determines if an action result is not allowed using its attached mapping.
 
     Args:
         result: The value returned by the action.

nemoguardrails/rails/llm/buffer.py

@@ -16,7 +16,7 @@
 from abc import ABC, abstractmethod
 from typing import AsyncGenerator, List, Tuple
 
-from nemoguardrails.rails.llm.config import OutputRailsStreamingConfig, RailsConfig
+from nemoguardrails.rails.llm.config import OutputRailsStreamingConfig
 
 
 class BufferStrategy(ABC):
@@ -104,16 +104,5 @@ def generate_chunk_str(self, buffer, current_index) -> str:
 
 def get_buffer_strategy(config: OutputRailsStreamingConfig) -> BufferStrategy:
     # TODO: use a factory function or class
-    # currently we only have SlidingWindo, in future we have a registry
+    # currently we only have SlidingWindow, in future we use a registry
     return SlidingWindow.from_config(config)
-
-
-def is_blocked(result: Tuple[bool, str]) -> bool:
-    """Check output rials status."""
-    # result is a tuple of (return_value, success|failure)
-    # return_value is not unified among all the actions
-    # sometimes it is a bool where True means allowed, sometimes True means blocked
-    # sometimes it is a score where allowed/blocked is dictated in flows.co
-    # lack of stable interface for output rails cause this issue
-    # for self_check_output following holds
-    return not result[0]

nemoguardrails/rails/llm/llmrails.py

@@ -31,7 +31,7 @@
 
 from nemoguardrails.actions.llm.generation import LLMGenerationActions
 from nemoguardrails.actions.llm.utils import get_colang_history
-from nemoguardrails.actions.output_mapping import should_block_output
+from nemoguardrails.actions.output_mapping import is_output_blocked
 from nemoguardrails.actions.v2_x.generation import LLMGenerationActionsV2dotx
 from nemoguardrails.colang import parse_colang_file
 from nemoguardrails.colang.v1_0.runtime.flows import compute_context
@@ -1303,7 +1303,7 @@ def _update_explain_info():
                 action_func = self.runtime.action_dispatcher.get_action(action_name)
 
                 # Use the mapping to decide if the result indicates blocked content.
-                if should_block_output(result, action_func):
+                if is_output_blocked(result, action_func):
                     # TODO: while whitespace issue is fixed, remove the space from below
                     yield " {DATA: STOP}"
                     return

tests/test_actions_output_mapping.py

@@ -19,7 +19,7 @@
 from nemoguardrails.actions import action
 from nemoguardrails.actions.output_mapping import (
     default_output_mapping,
-    should_block_output,
+    is_output_blocked,
 )
 
 # Tests for default_output_mapping
@@ -70,21 +70,21 @@ def test_should_block_output_with_tuple_result_and_mapping():
     # Test should_block_output when the result is a tuple and the dummy mapping is used.
     # When the first element equals "block", we expect True.
     result = ("block",)
-    assert should_block_output(result, dummy_action) is True
+    assert is_output_blocked(result, dummy_action) is True
 
     # When the result is not "block", we expect False.
     result = ("allow",)
-    assert should_block_output(result, dummy_action) is False
+    assert is_output_blocked(result, dummy_action) is False
 
 
 def test_should_block_output_with_non_tuple_result_and_mapping():
     # Test should_block_output when the result is not a tuple.
     # The function should wrap it into a tuple.
     result = "block"
-    assert should_block_output(result, dummy_action) is True
+    assert is_output_blocked(result, dummy_action) is True
 
     result = "allow"
-    assert should_block_output(result, dummy_action) is False
+    assert is_output_blocked(result, dummy_action) is False
 
 
 def test_should_block_output_without_action_meta():
@@ -98,9 +98,9 @@ def action_without_meta(res):
         del action_without_meta.action_meta
 
     # Test with a boolean: default_output_mapping for True is False and for False is True.
-    assert should_block_output(True, action_without_meta) is False
-    assert should_block_output(False, action_without_meta) is True
+    assert is_output_blocked(True, action_without_meta) is False
+    assert is_output_blocked(False, action_without_meta) is True
 
     # Test with a numeric value: block if < 0.5.
-    assert should_block_output(0.4, action_without_meta) is True
-    assert should_block_output(0.6, action_without_meta) is False
+    assert is_output_blocked(0.4, action_without_meta) is True
+    assert is_output_blocked(0.6, action_without_meta) is False