-
Notifications
You must be signed in to change notification settings - Fork 543
feat(tool-rails): add support for tool input rails and validation #1386
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
base: feat/tool-output-rails
Are you sure you want to change the base?
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.
Pull Request Overview
This PR implements comprehensive tool input rails functionality to validate and secure tool messages before they are processed by the LLM.
- Added
UserToolMessages
event processing with grouped tool message handling - Fixed message-to-event conversion for proper tool message support in conversation flows
- Enhanced passthrough mode to preserve full conversation context including tool calls
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/test_tool_calls_event_extraction.py | Updates test for event name change from BotToolCall to BotToolCalls |
tests/test_tool_calling_passthrough_only.py | Updates test assertions and comments for event name change |
tests/test_input_tool_rails.py | Comprehensive test suite for tool input rails functionality |
tests/test_bot_tool_call_events.py | Updates comments to reflect event name change |
tests/runnable_rails/test_tool_calling.py | Updates test comment for event name change |
tests/runnable_rails/test_runnable_rails.py | Updates test assertion for improved passthrough mode behavior |
tests/input_tool_rails_actions.py | Test utility actions for tool input validation, safety checking, and sanitization |
nemoguardrails/rails/llm/llmrails.py | Core implementation for message-to-event conversion with tool message support |
nemoguardrails/rails/llm/llm_flows.co | Flow definitions for tool input rails processing |
nemoguardrails/integrations/langchain/runnable_rails.py | Enhanced message conversion with tool call support |
nemoguardrails/actions/llm/utils.py | Updated tool call extraction functions for new event name |
nemoguardrails/actions/llm/generation.py | Enhanced passthrough mode to preserve tool message context |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
0358cd7
to
3240dc9
Compare
df75862
to
56cb9dd
Compare
21e33e2
to
2f57ec4
Compare
44a25f3
to
c8a4959
Compare
ed234d6
to
4c34032
Compare
…ion and processing - Add UserToolMessages event handling and tool input rails processing - Fix message-to-event conversion to properly handle tool messages in conversation history - Preserve tool call context in passthrough mode by using full conversation history - Support tool_calls and tool message metadata in LangChain format conversion - Include comprehensive test suite for tool input rails functionality test(runnable_rails): fix prompt format in passthrough mode feat: support ToolMessage in message dicts refactor: rename BotToolCall to BotToolCalls
4c34032
to
c8ff064
Compare
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
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
raw_prompt = raw_llm_request.get() | ||
|
||
if raw_prompt is not None and isinstance(raw_prompt, list): |
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.
The raw_llm_request.get()
call and the logic for handling tool message context in passthrough mode lacks documentation. Consider adding comments explaining when this context variable is set and how it differs from the regular prompt handling.
Copilot uses AI. Check for mistakes.
$tool_message = $tool_messages[$i].content | ||
$tool_name = $tool_messages[$i].name | ||
if "tool_call_id" in $tool_messages[$i] | ||
$tool_call_id = $tool_messages[$i].tool_call_id | ||
else | ||
$tool_call_id = "" |
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.
[nitpick] The manual iteration and field access pattern is repeated throughout the code. Consider extracting this into a helper function or using a more structured approach to access tool message fields consistently.
Copilot uses AI. Check for mistakes.
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.
Looks good, approving. Please address feedback before merging. This is another >1k line PR which should be broken into smaller stacked PRs to review.
Please also run local integration-tests to make a few tool-calls to production LLMs to check this works before merging. There are also some Github CI tests failing, could you take a look before merging?
@@ -591,7 +591,7 @@ async def generate_user_intent( | |||
|
|||
if tool_calls: | |||
output_events.append( | |||
new_event_dict("BotToolCall", tool_calls=tool_calls) | |||
new_event_dict("BotToolCalls", tool_calls=tool_calls) |
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.
Could you use an enum for these events? I'm guessing there's only a static limited number that are known ahead of time
AIMessage(content=msg["content"], tool_calls=tool_calls) | ||
) | ||
else: | ||
messages.append(AIMessage(content=msg["content"])) | ||
elif msg_type == "system": |
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.
Also need to add developer
which OpenAI uses rather than system
for models aftero1
@@ -130,6 +130,40 @@ define parallel extension flow process bot tool call | |||
create event StartToolCallBotAction(tool_calls=$tool_calls) | |||
|
|||
|
|||
define parallel flow process user tool messages | |||
"""Run all the tool input rails on the tool messages.""" | |||
priority 200 |
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.
Why is this set to 200? Is the aim to run all tool calls before other LLM calls?
|
||
|
||
@action(is_system_action=True) | ||
async def validate_tool_input_safety( |
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.
nit: Rename to _is_tool_input_safe()
? Then the "polarity" of the bool return is clear
|
||
|
||
@action(is_system_action=True) | ||
async def self_check_tool_input( |
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.
nit: Rename to is_tool_input_safe_self_check()
or similar? The polarity of the bool return type is clearer
|
||
log.debug(f"Validating safety of tool input from {tool_name}") | ||
|
||
dangerous_patterns = [ |
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.
Are we recommending using these in production? Seems like there might be valid uses for an api_key
argument in a tool call for example
PR Description
Tool Input Rails Implementation
This PR implements comprehensive tool input rails functionality to validate and secure tool messages before they are processed by the LLM.
Key Changes:
UserToolMessages
event processing with grouped tool message handlingBenefits: