feat(server): allow tool calls outside passthrough mode without dialog flows - #2188
Conversation
github-actions
Bot
added
status: needs triage
PR merge guidance@christinaexyou thanks for the PR. GitHub is currently blocking merge for one or more repository requirements:
Relevant guide: |
… flows Signed-off-by: Christina Xu <chrxu@redhat.com>
…are configured Signed-off-by: Christina Xu <chrxu@redhat.com>
aff6a45 to
9c8f260
Compare
Pouyanpi
changed the title
Pouyanpi
added
status: triaged
Greptile SummaryThis PR removes the
|
| Filename | Overview |
|---|---|
| nemoguardrails/server/api.py | Tool-call gating logic refactored: passthrough requirement removed, replaced with a two-stage check (streaming rejection, then dialog-flow detection). Minor style: missing blank line before try:. |
| nemoguardrails/actions/llm/generation.py | Removed passthrough guard around get_and_clear_tool_calls_contextvar(); BotToolCalls is now emitted unconditionally when tool calls are present, regardless of passthrough setting. |
| tests/integrations/langchain/test_tool_calling_passthrough.py | New test file covering passthrough and non-passthrough tool-calling. test_no_tool_calls_creates_bot_message_in_non_passthrough is broken: bound-mock replacement is incomplete so tool-calls response still fires and BotMessage is never verified. Several docstrings still describe the old 'ignored' behavior. |
| tests/server/test_api.py | Server-level tests updated to reflect new gating rules: passthrough no longer required, tools accepted for non-dialog-flow configs, rejected for streaming or dialog-flow configs. Assertions updated from 'passthrough' to 'dialog flows' / 'non-streaming'. |
| tests/test_tool_calling_passthrough_integration.py | Mock patch targets updated from nemoguardrails.actions.llm.utils to nemoguardrails.actions.llm.generation (correct after import move); new TestToolCallingNonPassthroughIntegration class added to exercise the non-passthrough path end-to-end. |
| tests/test_generation_equivalence.py | Two new tests added: test_non_passthrough_tool_calls verifies BotToolCalls in the general (non-passthrough) path; test_tools_not_supported_when_dialog_flows_are_enabled confirms tool calls are dropped when dialog flows exist. |
| docs/integration/tools-integration.mdx | Documentation updated: passthrough no longer described as required for tool calls; new paragraph explains the dialog-flow exclusion condition. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /v1/chat/completions] --> B{tools / tool_choice / parallel_tool_calls requested?}
B -- No --> G[Proceed to generate]
B -- Yes --> C{stream=true?}
C -- Yes --> D[422: non-streaming only]
C -- No --> E{has_dialog_flows?\nuser_messages OR single_call.enabled}
E -- Yes --> F[422: dialog flows not supported]
E -- No --> G
G --> H[generate_async]
H --> I[LLM call]
I --> J[get_and_clear_tool_calls_contextvar]
J --> K{tool_calls truthy?}
K -- Yes --> L[emit BotToolCalls event]
K -- No --> M[emit BotMessage event]
L --> N[Return response with tool_calls field]
M --> N
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[POST /v1/chat/completions] --> B{tools / tool_choice / parallel_tool_calls requested?}
B -- No --> G[Proceed to generate]
B -- Yes --> C{stream=true?}
C -- Yes --> D[422: non-streaming only]
C -- No --> E{has_dialog_flows?\nuser_messages OR single_call.enabled}
E -- Yes --> F[422: dialog flows not supported]
E -- No --> G
G --> H[generate_async]
H --> I[LLM call]
I --> J[get_and_clear_tool_calls_contextvar]
J --> K{tool_calls truthy?}
K -- Yes --> L[emit BotToolCalls event]
K -- No --> M[emit BotMessage event]
L --> N[Return response with tool_calls field]
M --> N
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
tests/integrations/langchain/test_tool_calling_passthrough.py:218-248
**No-tool-calls path is never exercised — test passes for the wrong reason**
The test intends to verify that when the LLM returns no tool calls, a `BotMessage` event is emitted. However, `get_bound_llm_magic_mock` routes actual calls through a _bound_ mock (`mock_llm.bind.return_value.ainvoke`), not through `mock_llm.ainvoke` directly. The test only reassigns `mock_llm_with_tool_calls.ainvoke.return_value` and `mock_llm_with_tool_calls.invoke.return_value`, leaving the bound mock's `ainvoke.return_value` pointing at the original fixture's tool-calls `AIMessage`. The `LangChainLLMAdapter` calls `llm.bind(...).ainvoke(...)`, so it still receives the tool-calls response, `tool_calls_var` gets repopulated, and `BotToolCalls` is emitted — which is why the assertion passes.
The stated negative path (`BotMessage` when `tool_calls_var` is `None`) is never actually exercised. To fix, also clear the bound mock: `mock_llm_with_tool_calls.bind.return_value.ainvoke.return_value = mock_response_no_tools` and update the assertion to `"BotMessage"`.
### Issue 2 of 3
tests/integrations/langchain/test_tool_calling_passthrough.py:251-253
**Stale docstrings contradict the PR's new behavior**
Multiple methods in `TestToolCallingNonPassthrough` carry docstrings from the old "ignored" behavior: `test_tool_calls_non_passthrough_mode` (line 199), `test_tool_calls_with_prompt_mode_non_passthrough` (line 252), and `test_complex_tool_calls_non_passthrough` all say "tool calls are ignored when not in passthrough mode." The PR explicitly changes this — tool calls are now surfaced in any config without dialog flows — so every assertion correctly checks for `BotToolCalls`. The docstrings need to be updated to match.
### Issue 3 of 3
nemoguardrails/server/api.py:566-568
Missing blank line between the `if tools_requested:` block and the `try:` block causes a minor PEP 8 violation and makes the guard clause less visually distinct from the next logical section.
```suggestion
),
)
try:
```
Reviews (1): Last reviewed commit: "docs(tool-calling): passthrough is not r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -192,20 +236,106 @@ async def test_no_tool_calls_creates_bot_message_in_passthrough(self, config_pas | |||
| context = {} | |||
|
|
|||
| result = await generation_actions.generate_user_intent( | |||
| events=events, context=context, config=config_passthrough | |||
| events=events, context=context, config=config_no_passthrough | |||
| ) | |||
|
|
|||
| assert len(result.events) == 1 | |||
| assert result.events[0]["type"] == "BotMessage" | |||
| assert result.events[0]["type"] == "BotToolCalls" | |||
| stored = result.events[0]["tool_calls"] | |||
| assert len(stored) == 1 | |||
| assert stored[0]["function"]["name"] == "test_tool" | |||
| assert stored[0]["function"]["arguments"] == {"param": "value"} | |||
| assert stored[0]["id"] == "call_123" | |||
There was a problem hiding this comment.
The test intends to verify that when the LLM returns no tool calls, a BotMessage event is emitted. However, get_bound_llm_magic_mock routes actual calls through a bound mock (mock_llm.bind.return_value.ainvoke), not through mock_llm.ainvoke directly. The test only reassigns mock_llm_with_tool_calls.ainvoke.return_value and mock_llm_with_tool_calls.invoke.return_value, leaving the bound mock's ainvoke.return_value pointing at the original fixture's tool-calls AIMessage. The LangChainLLMAdapter calls llm.bind(...).ainvoke(...), so it still receives the tool-calls response, tool_calls_var gets repopulated, and BotToolCalls is emitted — which is why the assertion passes.
The stated negative path (BotMessage when tool_calls_var is None) is never actually exercised. To fix, also clear the bound mock: mock_llm_with_tool_calls.bind.return_value.ainvoke.return_value = mock_response_no_tools and update the assertion to "BotMessage".
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/integrations/langchain/test_tool_calling_passthrough.py
Line: 218-248
Comment:
**No-tool-calls path is never exercised — test passes for the wrong reason**
The test intends to verify that when the LLM returns no tool calls, a `BotMessage` event is emitted. However, `get_bound_llm_magic_mock` routes actual calls through a _bound_ mock (`mock_llm.bind.return_value.ainvoke`), not through `mock_llm.ainvoke` directly. The test only reassigns `mock_llm_with_tool_calls.ainvoke.return_value` and `mock_llm_with_tool_calls.invoke.return_value`, leaving the bound mock's `ainvoke.return_value` pointing at the original fixture's tool-calls `AIMessage`. The `LangChainLLMAdapter` calls `llm.bind(...).ainvoke(...)`, so it still receives the tool-calls response, `tool_calls_var` gets repopulated, and `BotToolCalls` is emitted — which is why the assertion passes.
The stated negative path (`BotMessage` when `tool_calls_var` is `None`) is never actually exercised. To fix, also clear the bound mock: `mock_llm_with_tool_calls.bind.return_value.ainvoke.return_value = mock_response_no_tools` and update the assertion to `"BotMessage"`.
How can I resolve this? If you propose a fix, please make it concise.| async def test_tool_calls_with_prompt_mode_non_passthrough(self, config_no_passthrough, mock_llm_with_tool_calls): | ||
| """Test that tool calls are ignored when not in passthrough mode.""" | ||
| tool_calls = [ |
There was a problem hiding this comment.
Multiple methods in TestToolCallingNonPassthrough carry docstrings from the old "ignored" behavior: test_tool_calls_non_passthrough_mode (line 199), test_tool_calls_with_prompt_mode_non_passthrough (line 252), and test_complex_tool_calls_non_passthrough all say "tool calls are ignored when not in passthrough mode." The PR explicitly changes this — tool calls are now surfaced in any config without dialog flows — so every assertion correctly checks for BotToolCalls. The docstrings need to be updated to match.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/integrations/langchain/test_tool_calling_passthrough.py
Line: 251-253
Comment:
**Stale docstrings contradict the PR's new behavior**
Multiple methods in `TestToolCallingNonPassthrough` carry docstrings from the old "ignored" behavior: `test_tool_calls_non_passthrough_mode` (line 199), `test_tool_calls_with_prompt_mode_non_passthrough` (line 252), and `test_complex_tool_calls_non_passthrough` all say "tool calls are ignored when not in passthrough mode." The PR explicitly changes this — tool calls are now surfaced in any config without dialog flows — so every assertion correctly checks for `BotToolCalls`. The docstrings need to be updated to match.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ), | ||
| ) | ||
| try: |
There was a problem hiding this comment.
if tools_requested: block and the try: block causes a minor PEP 8 violation and makes the guard clause less visually distinct from the next logical section.
| ), | |
| ) | |
| try: | |
| ), | |
| ) | |
| try: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/server/api.py
Line: 566-568
Comment:
Missing blank line between the `if tools_requested:` block and the `try:` block causes a minor PEP 8 violation and makes the guard clause less visually distinct from the next logical section.
```suggestion
),
)
try:
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
… flows
Description
Tool calls were previously gated on passthrough mode. They are now allowed
in any config as long as no dialog flows are defined (
user_messagesorsingle_call dialog rails). Streaming requests still rejecttools/tool_choice/parallel_tool_calls. We explicitly reject tool call requests if dialog flows or streaming is enabled in the config.Related Issue(s)
Verification
AI Assistance
Checklist