Agentic flow first #1776
Agentic flow first #1776
Conversation
WalkthroughThis pull request updates several core components of the system. The changes modify the user identifier handling for LLM predictions, add a new FastAPI endpoint to execute agent flows, and enhance URL preparation logic. A new class is introduced to manage chatbot flow with methods for slot loading, rule execution, and event handling. Additionally, logging in the language model processor is streamlined, the tracker database URL is updated, and new tests are added to verify error handling and agent flow execution. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as WebClient Router
participant AF as AgenticFlow
C->>R: Send execute_flow request (AgenticFlowRequest, bot, user)
R->>AF: Instantiate AgenticFlow(bot, slot_vals, sender_id)
AF->>AF: Execute rule (execute_rule)
AF-->>R: Return execution result
R-->>C: Return flow execution response
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
kairon/shared/chat/agent/agent_flow.py (4)
14-15: Remove unused imports.Static analysis suggests that these imports are never referenced. Removing them helps maintain a clean codebase and avoids confusion.
Apply this diff to remove the unused imports:
- from kairon.history.processor import HistoryProcessor - from kairon.shared.admin.data_objects import BotSecrets🧰 Tools
🪛 Ruff (0.8.2)
14-14:
kairon.history.processor.HistoryProcessorimported but unusedRemove unused import:
kairon.history.processor.HistoryProcessor(F401)
15-15:
kairon.shared.admin.data_objects.BotSecretsimported but unusedRemove unused import:
kairon.shared.admin.data_objects.BotSecrets(F401)
21-21: Remove unused import.
bot_idis not used anywhere in the file. Consider removing it to reduce clutter and eliminate any confusion over its purpose.- from tests.unit_test.live_agent_test import bot_id🧰 Tools
🪛 Ruff (0.8.2)
21-21:
tests.unit_test.live_agent_test.bot_idimported but unusedRemove unused import:
tests.unit_test.live_agent_test.bot_id(F401)
99-99: Raise exceptions from the original error.Raising exceptions with
from ehelps preserve the traceback chain for better debugging and clarity in exception handling.- raise AppException(f"Error in loading slots: {e}") + raise AppException(f"Error in loading slots: {e}") from e - raise AppException(e) + raise AppException(e) from e - raise AppException(f"Error in executing action [{event['name']}]") + raise AppException(f"Error in executing action [{event['name']}]") from eAlso applies to: 118-118, 219-219
🧰 Tools
🪛 Ruff (0.8.2)
99-99: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
237-237: Simplify iteration over dictionary.Instead of iterating over
dict.keys(), iterate directly over the dictionary for cleaner code.- for name in self.fake_tracker.slots.keys(): + for name in self.fake_tracker.slots:🧰 Tools
🪛 Ruff (0.8.2)
237-237: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
tests/unit_test/chat/agentic_flow_test.py (1)
3-3: Remove unused imports to improve code cleanliness.The following imports are not used in the code:
MagicMockfromunittest.mockEventfromrasa.shared.core.eventsMailMessagefromimap_toolskairon.shared.chat.agent.agent_flow-from unittest.mock import patch, MagicMock +from unittest.mock import patch -from rasa.shared.core.events import ( - Event, - BotUttered, -) +from rasa.shared.core.events import BotUttered -from imap_tools import MailMessage -import kairon.shared.chat.agent.agent_flowAlso applies to: 7-7, 12-12, 16-16
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kairon/actions/definitions/prompt.py(1 hunks)kairon/chat/routers/web_client.py(2 hunks)kairon/shared/actions/utils.py(1 hunks)kairon/shared/chat/agent/agent_flow.py(1 hunks)kairon/shared/chat/models.py(2 hunks)kairon/shared/llm/processor.py(0 hunks)tests/unit_test/chat/agentic_flow_test.py(1 hunks)
💤 Files with no reviewable changes (1)
- kairon/shared/llm/processor.py
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/chat/routers/web_client.py
98-98: Do not perform function call Path in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
99-99: Do not perform function call Security in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
tests/unit_test/chat/agentic_flow_test.py
3-3: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
7-7: rasa.shared.core.events.Event imported but unused
Remove unused import: rasa.shared.core.events.Event
(F401)
12-12: imap_tools.MailMessage imported but unused
Remove unused import: imap_tools.MailMessage
(F401)
16-16: kairon.shared.chat.agent.agent_flow imported but unused
Remove unused import: kairon.shared.chat.agent.agent_flow
(F401)
21-21: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
kairon/shared/chat/agent/agent_flow.py
14-14: kairon.history.processor.HistoryProcessor imported but unused
Remove unused import: kairon.history.processor.HistoryProcessor
(F401)
15-15: kairon.shared.admin.data_objects.BotSecrets imported but unused
Remove unused import: kairon.shared.admin.data_objects.BotSecrets
(F401)
21-21: tests.unit_test.live_agent_test.bot_id imported but unused
Remove unused import: tests.unit_test.live_agent_test.bot_id
(F401)
99-99: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
118-118: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
219-219: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
237-237: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
kairon/shared/chat/models.py (1)
121-131: LGTM on the new request model.The
AgenticFlowRequestclass is well-structured, and the validation for thenamefield enhances robustness.kairon/chat/routers/web_client.py (1)
95-112: Endpoint for executing agent flows looks good.The new
execute_flowendpoint correctly initializes and invokesAgenticFlow. The usage ofAgenticFlowRequestis consistent with FastAPI best practices. The static analysis warnings about function calls in defaults are false positives in this context, as they align with typical FastAPI usage.🧰 Tools
🪛 Ruff (0.8.2)
98-98: Do not perform function call
Pathin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
99-99: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
kairon/actions/definitions/prompt.py (1)
75-77: LGTM! The user identifier retrieval logic looks good.The change correctly retrieves the user identifier from the
agentic_flow_bot_userslot with a fallback totracker.sender_id. This aligns with the newAgenticFlowfunctionality.tests/unit_test/chat/agentic_flow_test.py (1)
21-450: Great test coverage! The test suite is comprehensive and well-structured.The test suite thoroughly covers:
- Basic functionality (simple utterances)
- Error handling (non-existing rules, bot existence)
- Action execution
- Multiflow scenarios
- Slot influence and evaluation
- Chat history logging
A few suggestions to enhance the test suite further:
- Add negative test cases for invalid slot values
- Add test cases for concurrent execution scenarios
- Consider adding performance benchmarks for critical paths
🧰 Tools
🪛 Ruff (0.8.2)
21-21: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
kairon/shared/actions/utils.py (1)
397-400: LGTM! The URL preparation logic is more robust now.The change improves the handling of the
$INTENTplaceholder by:
- Checking if intent exists before using it
- Replacing with an empty string when intent is missing
This defensive programming approach prevents potential issues with undefined values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
kairon/shared/chat/agent/agent_flow.py (3)
75-100: Improve error handling in the load_slots method.The error handling can be improved by using
raise ... from eto preserve the exception chain.Apply this diff to improve error handling:
- raise AppException(f"Error in loading slots: {e}") + raise AppException(f"Error in loading slots: {e}") from e🧰 Tools
🪛 Ruff (0.8.2)
100-100: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
102-120: Improve error handling in the load_rule_events method.The error handling can be improved by using
raise ... from eto preserve the exception chain.Apply this diff to improve error handling:
- raise AppException(e) + raise AppException(e) from e🧰 Tools
🪛 Ruff (0.8.2)
119-119: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
203-225: Improve error handling in the execute_event method.The error handling can be improved by using
raise ... from eto preserve the exception chain.Apply this diff to improve error handling:
- raise AppException(f"Error in executing action [{event['name']}]") + raise AppException(f"Error in executing action [{event['name']}]") from e🧰 Tools
🪛 Ruff (0.8.2)
225-225: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
tests/unit_test/chat/agentic_flow_test.py (2)
1-16: Remove unused imports.The following imports are not used in the code:
MagicMockfromunittest.mockEventfromrasa.shared.core.eventsMailMessagefromimap_toolskairon.shared.chat.agent.agent_flowApply this diff to remove unused imports:
-from unittest.mock import patch, MagicMock +from unittest.mock import patch -from rasa.shared.core.events import ( - Event, - BotUttered, -) +from rasa.shared.core.events import BotUttered -from imap_tools import MailMessage from joblib.testing import fixture from mongoengine import connect, disconnect -import kairon.shared.chat.agent.agent_flow from kairon import Utility🧰 Tools
🪛 Ruff (0.8.2)
3-3:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
7-7:
rasa.shared.core.events.Eventimported but unusedRemove unused import:
rasa.shared.core.events.Event(F401)
12-12:
imap_tools.MailMessageimported but unusedRemove unused import:
imap_tools.MailMessage(F401)
16-16:
kairon.shared.chat.agent.agent_flowimported but unusedRemove unused import:
kairon.shared.chat.agent.agent_flow(F401)
21-21: Use uppercase for environment variables.Follow Python conventions by using uppercase for environment variables.
Apply this diff to use uppercase for environment variables:
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.2)
21-21: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/shared/chat/agent/agent_flow.py(1 hunks)tests/unit_test/chat/agentic_flow_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/shared/chat/agent/agent_flow.py
100-100: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
119-119: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
243-243: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/unit_test/chat/agentic_flow_test.py
3-3: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
7-7: rasa.shared.core.events.Event imported but unused
Remove unused import: rasa.shared.core.events.Event
(F401)
12-12: imap_tools.MailMessage imported but unused
Remove unused import: imap_tools.MailMessage
(F401)
16-16: kairon.shared.chat.agent.agent_flow imported but unused
Remove unused import: kairon.shared.chat.agent.agent_flow
(F401)
21-21: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
kairon/shared/chat/agent/agent_flow.py (2)
20-49: LGTM! Well-structured class with clear responsibilities.The class follows good design principles:
- Single responsibility principle
- Clear error handling and logging
- Maintainable slot type mapping
- Proper initialization and cleanup
51-71: LGTM! Well-implemented initialization method.The method has:
- Proper bot validation
- Good error handling
- UUID for sender ID
- Complete attribute initialization
tests/unit_test/chat/agentic_flow_test.py (1)
285-451: LGTM! Well-structured test methods with comprehensive coverage.The test methods have:
- Clear naming conventions
- Proper test isolation
- Proper cleanup
- Comprehensive assertions
- Proper mocking
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/unit_test/chat/agentic_flow_test.py (4)
1-17: Unused imports detected.The following imports are not used in the code:
unittest.mock.MagicMockrasa.shared.core.events.EventRemove the unused imports to improve code maintainability:
-from unittest.mock import patch, MagicMock +from unittest.mock import patch -from rasa.shared.core.events import ( - Event, - BotUttered, -) +from rasa.shared.core.events import BotUttered🧰 Tools
🪛 Ruff (0.8.2)
3-3:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
7-7:
rasa.shared.core.events.Eventimported but unusedRemove unused import:
rasa.shared.core.events.Event(F401)
28-48: Setup method needs improvement.The setup method uses a lowercase environment variable name and includes a debug print statement.
- Use uppercase for environment variables
- Remove debug print statement
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" Utility.load_environment() Utility.load_system_metadata() connect(**Utility.mongoengine_connection()) BotSettings.objects(user="af_channel_test_user_acc").delete() Bot.objects(user="af_channel_test_user_acc").delete() Account.objects(user="af_channel_test_user_acc").delete() Rules.objects(user="af_channel_test_user_acc").delete() Responses.objects(user="af_channel_test_user_acc").delete() Slots.objects(user="af_channel_test_user_acc").delete() MultiflowStories.objects(user="af_channel_test_user_acc").delete() a = Account.objects.create(name="af_channel_test_user_acc", user="af_channel_test_user_acc") bot = Bot.objects.create(name="af_channel_test_bot", user="af_channel_test_user_acc", status=True, account=a.id) pytest.af_test_bot = str(bot.id) pytest.af_test_user = 'af_channel_test_user_acc' BotSettings(bot=pytest.af_test_bot, user="af_channel_test_user_acc").save() -print("setup")🧰 Tools
🪛 Ruff (0.8.2)
30-30: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
384-385: Debug print statement in test method.Remove debug print statement to maintain clean test output.
-print(flow.executed_actions) assert flow.executed_actions == ['py_multi', 'py_ans_a']
393-394: Test assertion could be more specific.The test asserts the length of slots but doesn't verify the actual slot values.
Consider adding assertions for the actual slot values:
slots = flow.load_slots() -print(slots) assert len(slots) == 4 +assert all(isinstance(slot, Slots) for slot in slots) slot_names = [slot.name for slot in slots] assert "trigger_conditional" in slot_names assert "subject" in slot_names assert "order" in slot_names assert "agentic_flow_bot_user" in slot_namestests/unit_test/chat/chat_test.py (2)
248-249: Debug print statement in test method.Remove debug print statement to maintain clean test output.
-print(log) assert log['type'] == 'whatsapp'
625-626: Debug print statement in test method.Remove debug print statement to maintain clean test output.
-print(history, message) assert len(history) == 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
system.yaml(1 hunks)tests/unit_test/chat/agentic_flow_test.py(1 hunks)tests/unit_test/chat/chat_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/chat/agentic_flow_test.py
3-3: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
7-7: rasa.shared.core.events.Event imported but unused
Remove unused import: rasa.shared.core.events.Event
(F401)
30-30: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
system.yaml (1)
177-177: Review Change: Updated Tracker URL DefaultThe default value for the tracker URL has been updated to
"mongodb://localhost:27017/test_conversations"on line 177. This change indicates that the tracker will now connect to a test-specific database instead of the previously used default (commonly"mongodb://localhost:27017/rasa"or a production database).
- Action Required: Ensure that this change is intentional. Since the overall environment is set to
prod(line 1), double-check that using a test database for conversation tracking fits the deployment strategy and does not inadvertently affect production data.- Consideration: Verify that appropriate environment variable overrides (i.e.,
TRACKER_URL) are applied during deployments to avoid potential misconfigurations.tests/unit_test/chat/agentic_flow_test.py (1)
291-298: Test coverage looks comprehensive.The test suite covers various scenarios including:
- Simple utterances
- Non-existing rules
- Bot existence checks
- Rules with actions
- Rules with multiflow
- Slot influence
- Chat history logging
This is a good example of thorough test coverage.
tests/unit_test/chat/chat_test.py (2)
40-52: Test case validates invalid channel configurations.Good test coverage for invalid channel configurations:
- Tests custom channel type validation
- Tests missing bot_user_oAuth_token
- Uses proper error message assertions
🧰 Tools
🪛 Gitleaks (8.21.2)
44-44: Identified a Slack Bot token, which may compromise bot integrations and communication channel security.
(slack-bot-token)
45-45: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
50-50: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1042-1067: Well-structured test for message processing.The test case:
- Uses proper mocking for dependencies
- Tests multiple messages
- Verifies response format
- Checks metadata handling
- Validates call counts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/unit_test/agentic_flow_test.py (8)
1-14: Clean up unused imports.Remove the following unused imports to keep the codebase clean:
MagicMockfromunittest.mockEventfromrasa.shared.core.events-from unittest.mock import patch, MagicMock +from unittest.mock import patch -from rasa.shared.core.events import ( - Event, - BotUttered, -) +from rasa.shared.core.events import BotUttered🧰 Tools
🪛 Ruff (0.8.2)
3-3:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
7-7:
rasa.shared.core.events.Eventimported but unusedRemove unused import:
rasa.shared.core.events.Event(F401)
30-30: Follow environment variable naming convention.Environment variables should be in uppercase.
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.2)
30-30: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
28-288: Add docstring to the setup method.The setup method is well-structured but lacks documentation explaining its purpose and the test data being initialized.
@pytest.fixture(autouse=True, scope='function') def setup(self): + """ + Setup method to initialize test environment and data. + + This fixture: + 1. Configures environment variables + 2. Sets up MongoDB connection + 3. Cleans up existing test data + 4. Creates test bot, account, rules, responses, slots, and multiflow stories + 5. Cleans up after tests + """ os.environ["system_file"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.2)
30-30: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
291-388: Add docstrings to test methods.Test methods should have docstrings explaining the test scenario and expected behavior.
Example for one test method:
@pytest.mark.asyncio @patch('kairon.shared.chat.agent.agent_flow.AgenticFlow.log_chat_history') async def test_simple_utter(self, patch_log_chat_history): + """ + Test execution of a simple utterance rule. + + Verifies that: + 1. The rule executes successfully + 2. Returns expected response text + 3. Chat history is logged + """ print(pytest.af_test_bot)
390-399: Remove debug print statement and optimize slot loading test.The test_loading_slots method contains a debug print statement and could be more efficient using set operations.
def test_loading_slots(self): flow = AgenticFlow(pytest.af_test_bot) slots = flow.load_slots() - print(slots) assert len(slots) == 4 - slot_names = [slot.name for slot in slots] - assert "trigger_conditional" in slot_names - assert "subject" in slot_names - assert "order" in slot_names - assert "agentic_flow_bot_user" in slot_names + expected_slots = {"trigger_conditional", "subject", "order", "agentic_flow_bot_user"} + actual_slots = {slot.name for slot in slots} + assert expected_slots == actual_slots
384-384: Remove debug print statement.Remove the debug print statement in test_execute__rule_with_multiflow_slot_influence2.
- print(flow.executed_actions)
393-393: Remove debug print statement.Remove the debug print statement in test_loading_slots.
- print(slots)
294-294: Remove debug print statement.Remove the debug print statement in test_simple_utter.
- print(pytest.af_test_bot)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/agentic_flow_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unit_test/agentic_flow_test.py
3-3: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
7-7: rasa.shared.core.events.Event imported but unused
Remove unused import: rasa.shared.core.events.Event
(F401)
30-30: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (python)
- GitHub Check: Python CI
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
kairon/shared/chat/agent/agent_flow.py (2)
20-23: Consider breaking down the class into smaller, focused components.The
AgenticFlowclass has multiple responsibilities including slot management, rule execution, event handling, and chat history logging. Consider splitting these into separate components to improve maintainability and testability.
243-244: Optimize dictionary iteration.When iterating over dictionary keys, you can directly iterate over the dictionary instead of using
.keys().Apply this diff:
- for name in self.fake_tracker.slots.keys(): - slot = self.fake_tracker.slots[name] + for name, slot in self.fake_tracker.slots.items():🧰 Tools
🪛 Ruff (0.8.2)
243-243: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
tests/unit_test/agentic_flow_test.py (4)
3-3: Remove unused imports.The following imports are not used in the code:
MagicMockfromunittest.mockEventfromrasa.shared.core.eventsAlso applies to: 7-7
🧰 Tools
🪛 Ruff (0.8.2)
3-3:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
28-28: Follow Python naming conventions for environment variables.Environment variables should be capitalized. Replace
system_filewithSYSTEM_FILE.Apply this diff:
- os.environ["system_file"] = "./tests/testing_data/system.yaml" + os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Also applies to: 40-40
🧰 Tools
🪛 Ruff (0.8.2)
28-28: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
386-386: Remove debug print statement.Remove the debug print statement as it's not needed in test code.
Apply this diff:
- print(flow.executed_actions)
395-395: Remove debug print statement.Remove the debug print statement as it's not needed in test code.
Apply this diff:
- print(slots)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/shared/chat/agent/agent_flow.py(1 hunks)tests/unit_test/agentic_flow_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
kairon/shared/chat/agent/agent_flow.py
100-100: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
119-119: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
243-243: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/unit_test/agentic_flow_test.py
3-3: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
7-7: rasa.shared.core.events.Event imported but unused
Remove unused import: rasa.shared.core.events.Event
(F401)
28-28: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
40-40: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
|
|
||
| class AgenticFlow: | ||
| mongo_processor = MongoProcessor() | ||
| chat_history_client = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using class-level shared database connection.
The chat_history_client is shared across all instances which could lead to connection leaks. Consider managing the connection lifecycle per instance or using a connection pool.
| except DoesNotExist as e: | ||
| raise AppException(e) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception context when re-raising exceptions.
When re-raising exceptions, use raise ... from e to preserve the original exception context for better debugging.
Apply this diff:
- raise AppException(e)
+ raise AppException(e) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except DoesNotExist as e: | |
| raise AppException(e) | |
| except DoesNotExist as e: | |
| - raise AppException(e) | |
| + raise AppException(e) from e |
🧰 Tools
🪛 Ruff (0.8.2)
119-119: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| except Exception as e: | ||
| logger.error(f"Error in loading slots: {e}") | ||
| raise AppException(f"Error in loading slots: {e}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception context when re-raising exceptions.
When re-raising exceptions, use raise ... from e to preserve the original exception context for better debugging.
Apply this diff:
- raise AppException(f"Error in loading slots: {e}")
+ raise AppException(f"Error in loading slots: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error(f"Error in loading slots: {e}") | |
| raise AppException(f"Error in loading slots: {e}") | |
| except Exception as e: | |
| logger.error(f"Error in loading slots: {e}") | |
| raise AppException(f"Error in loading slots: {e}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
100-100: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| except Exception as e: | ||
| logger.error(f"Error in executing action [{event['name']}]: {e}") | ||
| raise AppException(f"Error in executing action [{event['name']}]") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve exception context when re-raising exceptions.
When re-raising exceptions, use raise ... from e to preserve the original exception context for better debugging.
Apply this diff:
- raise AppException(f"Error in executing action [{event['name']}]")
+ raise AppException(f"Error in executing action [{event['name']}]") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error(f"Error in executing action [{event['name']}]: {e}") | |
| raise AppException(f"Error in executing action [{event['name']}]") | |
| except Exception as e: | |
| logger.error(f"Error in executing action [{event['name']}]: {e}") | |
| raise AppException(f"Error in executing action [{event['name']}]") from e |
🧰 Tools
🪛 Ruff (0.8.2)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/integration_test/chat_service_test.py (1)
4059-4075: Add test cases for error scenarios.The test case covers the happy path well, but consider adding tests for:
- Invalid flow name
- Flow execution failures
- Missing required parameters
- Authorization failures
Would you like me to help generate additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kairon/actions/definitions/prompt.py(1 hunks)tests/integration_test/chat_service_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kairon/actions/definitions/prompt.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/integration_test/chat_service_test.py
13-13: kairon.shared.chat.agent.agent_flow imported but unused
Remove unused import: kairon.shared.chat.agent.agent_flow
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
tests/integration_test/chat_service_test.py (1)
4057-4058: LGTM! Optimization to exit loop early.The addition of the break statement is a good optimization since we only need to verify the first matching bot event.
Summary by CodeRabbit
New Features
AgenticFlowclass for improved management of chatbot actions and responses.AgenticFlowRequest, for better handling of agent flow requests.Bug Fixes
Tests
AgenticFlowclass to ensure robust error handling and functionality.