-
Notifications
You must be signed in to change notification settings - Fork 798
feat(server): allow tool calls outside passthrough mode without dialog flows #2188
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: pouyanpi/improve-generate-colang
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Test that tool calling ONLY works in passthrough mode.""" | ||
| """Test tool-calling behavior across passthrough and non-dialog-flow general modes.""" | ||
|
|
||
| from unittest.mock import MagicMock | ||
|
|
||
|
|
@@ -94,16 +94,12 @@ def config_no_passthrough(): | |
|
|
||
|
|
||
| class TestToolCallingPassthroughOnly: | ||
| """Test that tool calling only works in passthrough mode.""" | ||
| """Test that tool calling works in passthrough mode.""" | ||
|
|
||
| def test_config_passthrough_true(self, config_passthrough): | ||
| """Test that passthrough config is correctly set.""" | ||
| assert config_passthrough.passthrough is True | ||
|
|
||
| def test_config_passthrough_false(self, config_no_passthrough): | ||
| """Test that non-passthrough config is correctly set.""" | ||
| assert config_no_passthrough.passthrough is False | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_tool_calls_work_in_passthrough_mode(self, config_passthrough, mock_llm_with_tool_calls): | ||
| """Test that tool calls create BotToolCalls events in passthrough mode.""" | ||
|
|
@@ -136,13 +132,54 @@ async def test_tool_calls_work_in_passthrough_mode(self, config_passthrough, moc | |
| assert len(result.events) == 1 | ||
| 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" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_tool_calls_ignored_in_non_passthrough_mode(self, config_no_passthrough, mock_llm_with_tool_calls): | ||
| async def test_no_tool_calls_creates_bot_message_in_passthrough(self, config_passthrough, mock_llm_with_tool_calls): | ||
| """Test that no tool calls creates BotMessage event even in passthrough mode.""" | ||
| tool_calls_var.set(None) | ||
|
|
||
| mock_response_no_tools = AIMessage(content="Regular text response") | ||
| mock_llm_with_tool_calls.ainvoke.return_value = mock_response_no_tools | ||
| mock_llm_with_tool_calls.invoke.return_value = mock_response_no_tools | ||
|
|
||
| generation_actions = LLMGenerationActions( | ||
| config=config_passthrough, | ||
| llm=LangChainLLMAdapter(mock_llm_with_tool_calls), | ||
| llm_task_manager=MagicMock(), | ||
| get_embedding_search_provider_instance=MagicMock(return_value=None), | ||
| ) | ||
|
|
||
| events = [{"type": "UserMessage", "text": "test"}] | ||
| context = {} | ||
|
|
||
| result = await generation_actions.generate_user_intent( | ||
| events=events, context=context, config=config_passthrough | ||
| ) | ||
|
|
||
| assert len(result.events) == 1 | ||
| assert result.events[0]["type"] == "BotMessage" | ||
|
|
||
| def test_llm_rails_integration_passthrough_mode(self, config_passthrough, mock_llm_with_tool_calls): | ||
| """Test LLMRails with passthrough mode allows tool calls.""" | ||
| rails = LLMRails(config=config_passthrough, llm=LangChainLLMAdapter(mock_llm_with_tool_calls)) | ||
|
|
||
| assert rails.config.passthrough is True | ||
|
|
||
|
|
||
| class TestToolCallingNonPassthrough: | ||
| """Test that tool calling works in non-passthrough mode.""" | ||
|
|
||
| def test_config_no_passthrough_true(self, config_no_passthrough): | ||
| """Test that non-passthrough config is correctly set.""" | ||
| assert config_no_passthrough.passthrough is False | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_tool_calls_non_passthrough_mode(self, config_no_passthrough, mock_llm_with_tool_calls): | ||
| """Test that tool calls are ignored when not in passthrough mode.""" | ||
| tool_calls = [ | ||
| { | ||
|
|
@@ -169,20 +206,27 @@ async def test_tool_calls_ignored_in_non_passthrough_mode(self, config_no_passth | |
| ) | ||
|
|
||
| assert len(result.events) == 1 | ||
| assert result.events[0]["type"] == "BotMessage" | ||
| assert "tool_calls" not in result.events[0] | ||
| 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" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_no_tool_calls_creates_bot_message_in_passthrough(self, config_passthrough, mock_llm_with_tool_calls): | ||
| """Test that no tool calls creates BotMessage event even in passthrough mode.""" | ||
| async def test_no_tool_calls_creates_bot_message_in_non_passthrough( | ||
| self, config_no_passthrough, mock_llm_with_tool_calls | ||
| ): | ||
| """Test that no tool calls creates BotMessage event even in non-passthrough mode.""" | ||
| tool_calls_var.set(None) | ||
|
|
||
| mock_response_no_tools = AIMessage(content="Regular text response") | ||
| mock_llm_with_tool_calls.ainvoke.return_value = mock_response_no_tools | ||
| mock_llm_with_tool_calls.invoke.return_value = mock_response_no_tools | ||
|
|
||
| generation_actions = LLMGenerationActions( | ||
| config=config_passthrough, | ||
| config=config_no_passthrough, | ||
| llm=LangChainLLMAdapter(mock_llm_with_tool_calls), | ||
| llm_task_manager=MagicMock(), | ||
| get_embedding_search_provider_instance=MagicMock(return_value=None), | ||
|
|
@@ -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" | ||
|
Comment on lines
218
to
+248
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The test intends to verify that when the LLM returns no tool calls, a The stated negative path ( Prompt To Fix With AIThis 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. |
||
|
|
||
| def test_llm_rails_integration_passthrough_mode(self, config_passthrough, mock_llm_with_tool_calls): | ||
| """Test LLMRails with passthrough mode allows tool calls.""" | ||
| rails = LLMRails(config=config_passthrough, llm=LangChainLLMAdapter(mock_llm_with_tool_calls)) | ||
| @pytest.mark.asyncio | ||
| 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 = [ | ||
|
Comment on lines
+251
to
+253
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Multiple methods in Prompt To Fix With AIThis 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! |
||
| { | ||
| "id": "call_123", | ||
| "type": "tool_call", | ||
| "name": "test_tool", | ||
| "args": {"param": "value"}, | ||
| } | ||
| ] | ||
| tool_calls_var.set(tool_calls) | ||
|
|
||
| assert rails.config.passthrough is True | ||
| generation_actions = LLMGenerationActions( | ||
| config=config_no_passthrough, | ||
| llm=LangChainLLMAdapter(mock_llm_with_tool_calls), | ||
| llm_task_manager=MagicMock(), | ||
| get_embedding_search_provider_instance=MagicMock(return_value=None), | ||
| ) | ||
|
|
||
| events = [{"type": "UserMessage", "text": "test"}] | ||
| context = {} | ||
|
|
||
| result = await generation_actions.generate_user_intent( | ||
| events=events, context=context, config=config_no_passthrough | ||
| ) | ||
| assert len(result.events) == 1 | ||
| 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" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_complex_tool_calls_non_passthrough(self, config_no_passthrough, mock_llm_with_tool_calls): | ||
| """Test that complex tool calls are ignored when not in passthrough mode.""" | ||
| tool_calls = [ | ||
| { | ||
| "id": "call_123", | ||
| "type": "tool_call", | ||
| "name": "test_tool", | ||
| "args": {"param": "value"}, | ||
| } | ||
| ] | ||
| tool_calls_var.set(tool_calls) | ||
| generation_actions = LLMGenerationActions( | ||
| config=config_no_passthrough, | ||
| llm=LangChainLLMAdapter(mock_llm_with_tool_calls), | ||
| llm_task_manager=MagicMock(), | ||
| get_embedding_search_provider_instance=MagicMock(return_value=None), | ||
| ) | ||
|
|
||
| def test_llm_rails_integration_non_passthrough_mode(self, config_no_passthrough, mock_llm_with_tool_calls): | ||
| """Test LLMRails without passthrough mode.""" | ||
| rails = LLMRails(config=config_no_passthrough, llm=LangChainLLMAdapter(mock_llm_with_tool_calls)) | ||
| events = [{"type": "UserMessage", "text": "test"}] | ||
| context = {} | ||
|
|
||
| assert rails.config.passthrough is False | ||
| result = await generation_actions.generate_user_intent( | ||
| events=events, context=context, config=config_no_passthrough | ||
| ) | ||
| assert len(result.events) == 1 | ||
| 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" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_and_clear_tool_calls_called_correctly_non_passthrough( | ||
| self, config_no_passthrough, mock_llm_with_tool_calls | ||
| ): | ||
| generation_actions = LLMGenerationActions( | ||
| config=config_no_passthrough, | ||
| llm=LangChainLLMAdapter(mock_llm_with_tool_calls), | ||
| llm_task_manager=MagicMock(), | ||
| get_embedding_search_provider_instance=MagicMock(return_value=None), | ||
| ) | ||
|
|
||
| events = [{"type": "UserMessage", "text": "test"}] | ||
| context = {} | ||
|
|
||
| result = await generation_actions.generate_user_intent( | ||
| events=events, context=context, config=config_no_passthrough | ||
| ) | ||
| assert len(result.events) == 1 | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if tools_requested:block and thetry:block causes a minor PEP 8 violation and makes the guard clause less visually distinct from the next logical section.Prompt To Fix With AI
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!