Callback pyscript final changes #1882
Conversation
…ipt-lambda to callback service and fixed test cases.
…ipt-lambda to callback service and fixed test cases.
WalkthroughThis pull request implements a major framework migration and functionality refactor. The asynchronous callback service is transitioned from FastAPI to BlackSheep with updated startup/shutdown procedures, routing, middleware (CORS, security, error handling), and JSON response formats. The processor now leverages a new CallbackUtility for remote script evaluation, while evaluator processing receives an async lifespan manager for MongoDB connections. Additional utility classes and methods are introduced for scheduled jobs, data operations, and API calls. Multiple integration and unit tests have been updated to support asynchronous operations and revised control flow. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as BlackSheep App
participant R as Router (pyscript_callback)
participant CU as CallbackUtility
participant DB as Database Services
C->>A: Send Request (/callback/d/{identifier}/{token})
A->>R: Route Request
R->>CU: Invoke pyscript_handler
CU->>DB: (Optional) Perform data operations / schedule job handling
CU-->>R: Return BSResponse with JSON error or success data
R-->>A: Forward Response
A-->>C: Deliver Final Response
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 9
🧹 Nitpick comments (24)
tests/unit_test/action/action_test.py (1)
1444-1450: Update test function name to match implementationThe test function is called
test_run_pyscript_action_without_pyscript_evaluator_url, but you've added a mock for the pyscript evaluator URL and configured a response for it. This creates a discrepancy between the test name and its actual behavior.Consider either:
- Renaming the test to better reflect its current purpose (e.g.,
test_run_pyscript_action_with_direct_http_call)- Or adjusting the test to properly verify behavior without a pyscript evaluator URL
This change appears to align with the framework migration described in the PR, but the test name should accurately reflect what's being tested.
tests/unit_test/callback_test.py (1)
384-397: Function is using a complex response structure.The test now mocks a much more structured HTTP-like response with status code, headers and a nested response body. This reflects an architectural change from using CloudUtility to CallbackUtility with a more comprehensive response format.
Consider adding test cases for different response codes and error scenarios to verify error handling in the processor. For example:
def test_run_pyscript_with_error_response(): mock_utility.environment = {'async_callback_action': {'pyscript': {'trigger_task': True}}} error_resp = { "statusCode": 400, "statusDescription": "400 Bad Request", "isBase64Encoded": False, "headers": { "Content-Type": "application/json" }, "body": {'error': 'Invalid script'} } mock_callback_utility.pyscript_handler.return_value = error_resp script = 'print("Hello, World!")' predefined_objects = {'bot_response': 'Test response'} with pytest.raises(AppException, match="Error in script execution"): CallbackProcessor.run_pyscript(script, predefined_objects)tests/unit_test/api/api_processor_test.py (1)
18-18: Remove unused importdisconnect.The
disconnectimport frommongoengineis not used in this file and should be removed to keep the code clean.-from mongoengine import connect, disconnect +from mongoengine import connect🧰 Tools
🪛 Ruff (0.8.2)
18-18:
mongoengine.disconnectimported but unusedRemove unused import:
mongoengine.disconnect(F401)
tests/integration_test/action_service_test.py (4)
1366-1373: Fix typo in mocked response data.The mock response contains a typo in the slots field: "langauge" should be "language".
"data": {"bot_response": "Successfully Evaluated the pyscript", - "slots": {"location": "Bangalore", "langauge": "Kannada"}}} + "slots": {"location": "Bangalore", "language": "Kannada"}}}
1366-1367: Consider extracting mock environment to avoid duplication.This mock environment configuration is duplicated in hunk 2 (lines 1442-1443). Consider extracting it to a helper method or fixture to maintain DRY principles.
1446-1446: Fix grammatical error in error message.The error message contains a grammatical error.
- json={"message": "Failed to evaluated the pyscript", "success": False, "error_code": 422, "data": None} + json={"message": "Failed to evaluate the pyscript", "success": False, "error_code": 422, "data": None}
1488-1488: Consider making error assertion more resilient.The assertion checks the exact error message, which includes the entire JSON response. This makes the test brittle to format changes. Consider verifying specific parts of the error message or parsing the JSON to check its structure.
- assert log['exception'] == "Pyscript evaluation failed: {'message': 'Failed to evaluated the pyscript', 'success': False, 'error_code': 422, 'data': None}" + import json + exception_msg = log['exception'] + assert "Pyscript evaluation failed" in exception_msg + # Extract and validate JSON part + json_str = exception_msg.split("Pyscript evaluation failed: ", 1)[1] + error_response = json.loads(json_str.replace("'", '"')) + assert error_response['success'] is False + assert error_response['error_code'] == 422kairon/shared/actions/utils.py (1)
685-689: Ensure robust exception handling for unexpected response shapes.
If the external service fails to returnerror_codeordatawith the expected structure, this logic could lead to uncaught errors or attribute lookups on None. Consider adding fallback checks or validation for the response object.kairon/async_callback/processor.py (1)
41-49: Handle dictionary keys safely.
Ensure thatresponsealways contains thestatusCodeandbodyfields before usage. You might consider a defensive check to avoid potentialKeyErrorif the structure changes.tests/integration_test/callback_service_test.py (2)
183-194: Consider removing debug print
print(json_response)(line 189) might clutter test logs in CI environments.
196-208: Duplicate assertion
Lines 206 and 207 both assertjson_response["error_code"] == 0, which is redundant.tests/unit_test/callback/pyscript_handler_test.py (1)
12-12: Use standardized environment variable
Ruff suggests usingSYSTEM_FILEinstead ofsystem_fileto conform to typical naming conventions for environment variables.🧰 Tools
🪛 Ruff (0.8.2)
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
kairon/async_callback/router/pyscript_callback.py (2)
23-24: Avoid calling.keys()when iterating.According to static analysis, you can simplify the dictionary iteration by dropping
.keys(). For example:- data['params'].update({key: request.query.get(key) for key in request.query.keys()}) + data['params'].update({key: request.query.get(key) for key in request.query})🧰 Tools
🪛 Ruff (0.8.2)
24-24: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
32-34: Remove unused exception variable.Variable
eis never used within theexceptblock. You could either referencee(e.g., by logging it) or remove the variable to satisfy linters and maintain clarity.🧰 Tools
🪛 Ruff (0.8.2)
32-32: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
kairon/shared/concurrency/actors/utils.py (2)
23-24: Consider renaming to match Python's standard.
srtptimeorsrtftimedeviate from the standard library'sstrptime/strftimenaming. If there's no specific need for this change, consider aligning naming conventions for clarity and maintenance.
163-165: Specify a timeout when making HTTP requests.Using
requests.requestwithout a timeout can lead to unbounded waits in production. Add a reasonabletimeoutto ensure the service remains responsive and fails gracefully on slow or unresponsive endpoints.http_response = requests.request( request_method.upper(), http_url, headers=headers, json=request_body + , timeout=10 )kairon/async_callback/utils.py (6)
46-57: Consider validating timezone-awareness.
timeval.utctimetuple()may behave unexpectedly iftimevalis a naive datetime. Optionally, ensure it's UTC or offset-aware.
58-131: Replaceprint()with a logger call and add error handling for DB operations.
Line 68 useslogger.info, which is inconsistent with the rest of the file. Also, consider gracefully handling potential insert failures before calling the event dispatcher.- print(f"bot: {bot}, name: {schedule_action}") + logger.info(f"bot: {bot}, name: {schedule_action}")
153-180: Check for missing or invalid EmailActionConfig.
Ifemail_action_configis not found,to_mongo()might fail. Add error handling for that scenario.
198-225: Restricting the requests module usage might be worth consideration.
Allowing script-basedrequestscalls from user-provided code could become an attack vector. Evaluate limiting external calls or using a sandbox.
226-251: Consider returning JSON responses inpyscript_handler.
The code sets"Content-Type": "text/html; charset=utf-8". Using JSON might be more consistent with an API-style response.
252-283: Use consistent exception classes.
Here, an unqualifiedExceptionis raised instead ofAppException. This inconsistency may complicate error handling across methods.tests/unit_test/actors/actors_test.py (2)
8-8: Remove unused import.
litellmis imported but never used.- import litellm🧰 Tools
🪛 Ruff (0.8.2)
8-8:
litellmimported but unusedRemove unused import:
litellm(F401)
376-377: Unused mock variables.
mock_get_llm_secretandmock_litellmare assigned but never utilized in the test. You can remove them to reduce clutter.🧰 Tools
🪛 Ruff (0.8.2)
376-376: Local variable
mock_get_llm_secretis assigned to but never usedRemove assignment to unused variable
mock_get_llm_secret(F841)
377-377: Local variable
mock_litellmis assigned to but never usedRemove assignment to unused variable
mock_litellm(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
kairon/async_callback/main.py(2 hunks)kairon/async_callback/processor.py(3 hunks)kairon/async_callback/router/pyscript_callback.py(1 hunks)kairon/async_callback/utils.py(1 hunks)kairon/evaluator/main.py(2 hunks)kairon/shared/actions/utils.py(1 hunks)kairon/shared/concurrency/actors/pyscript_runner.py(3 hunks)kairon/shared/concurrency/actors/utils.py(1 hunks)kairon/shared/data/constant.py(1 hunks)requirements/prod.txt(1 hunks)tests/integration_test/action_service_test.py(3 hunks)tests/integration_test/callback_service_test.py(5 hunks)tests/integration_test/evaluator_service_test.py(3 hunks)tests/unit_test/action/action_test.py(1 hunks)tests/unit_test/actors/actors_test.py(6 hunks)tests/unit_test/api/api_processor_test.py(3 hunks)tests/unit_test/callback/pyscript_handler_test.py(1 hunks)tests/unit_test/callback_test.py(1 hunks)tests/unit_test/evaluator/evaluator_test.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (8)
kairon/evaluator/main.py (4)
kairon/events/server.py (1)
lifespan(55-60)kairon/api/app/main.py (1)
lifespan(67-74)kairon/actions/server.py (1)
lifespan(95-103)kairon/chat/server.py (1)
lifespan(70-75)
kairon/shared/actions/utils.py (2)
kairon/shared/utils.py (1)
execute_http_request(1695-1791)tests/unit_test/utility_test.py (1)
execute_http_request(2266-2267)
kairon/async_callback/processor.py (1)
kairon/async_callback/utils.py (2)
CallbackUtility(40-316)pyscript_handler(227-250)
kairon/async_callback/main.py (1)
kairon/async_callback/processor.py (1)
async_callback(85-122)
kairon/shared/concurrency/actors/pyscript_runner.py (1)
kairon/shared/concurrency/actors/utils.py (6)
PyscriptUtility(20-173)srtptime(23-24)srtftime(27-28)url_parse_quote_plus(31-32)get_db_action_data(138-150)api_call(153-173)
kairon/async_callback/utils.py (1)
kairon/shared/actions/utils.py (2)
ActionUtility(35-1041)execute_http_request(95-139)
kairon/shared/concurrency/actors/utils.py (1)
kairon/shared/actions/utils.py (5)
ActionUtility(35-1041)get_payload(649-678)is_empty(419-428)prepare_url(382-416)prepare_request(158-179)
tests/integration_test/callback_service_test.py (4)
kairon/async_callback/router/pyscript_callback.py (1)
process_router_message(12-82)tests/integration_test/evaluator_service_test.py (1)
test_index(24-29)tests/integration_test/action_service_test.py (2)
test_index(69-73)test_healthcheck(76-80)tests/integration_test/services_test.py (1)
test_healthcheck(150-154)
🪛 Ruff (0.8.2)
kairon/async_callback/main.py
16-16: Redefinition of unused Response from line 11
(F811)
kairon/async_callback/router/pyscript_callback.py
24-24: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
32-32: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
tests/unit_test/actors/actors_test.py
8-8: litellm imported but unused
Remove unused import: litellm
(F401)
376-376: Local variable mock_get_llm_secret is assigned to but never used
Remove assignment to unused variable mock_get_llm_secret
(F841)
377-377: Local variable mock_litellm is assigned to but never used
Remove assignment to unused variable mock_litellm
(F841)
tests/unit_test/callback/pyscript_handler_test.py
12-12: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
kairon/shared/concurrency/actors/utils.py
129-129: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tests/unit_test/api/api_processor_test.py
18-18: mongoengine.disconnect imported but unused
Remove unused import: mongoengine.disconnect
(F401)
tests/integration_test/callback_service_test.py
10-10: Redefinition of unused TestClient from line 9
(F811)
🪛 Gitleaks (8.21.2)
tests/unit_test/actors/actors_test.py
117-117: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
161-161: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/unit_test/callback/pyscript_handler_test.py
528-528: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (64)
kairon/shared/data/constant.py (1)
233-233: LGTM - Well-defined constant for vector database suffixThe addition of the
QDRANT_SUFFIXconstant follows the established naming convention and formatting style of the file. This constant will likely be used for naming collections in the Qdrant vector database for FAQ embeddings.requirements/prod.txt (1)
73-73:❓ Verification inconclusive
Addition of BlackSheep dependency aligns with framework migration
The addition of
blacksheep==2.0.7is consistent with the PR objectives to migrate the callback service from FastAPI to BlackSheep.
🏁 Script executed:
#!/bin/bash # Check if there's a newer version of BlackSheep available pip index versions blacksheep --pre | head -5Length of output: 804
Action: Please verify the selected BlackSheep version in production requirements
It was noted that the dependency is now pinned to
blacksheep==2.0.7, which aligns with the migration from FastAPI to BlackSheep. However, our version check indicates that a newer version (2.1.0) is available. Please confirm if pinning to version 2.0.7 is intentional—for example, to ensure stability—or if an upgrade to 2.1.0 should be considered.
- Verify that version 2.0.7 is the intended choice.
- Consider adding a brief comment on the version selection rationale if outdated compared to the latest available release.
kairon/evaluator/main.py (3)
6-6: LGTM - Required imports for database connection lifecycle managementThe imports for
connect,disconnect, andasynccontextmanagerare necessary for implementing the FastAPI application lifecycle management.Also applies to: 14-14
57-57: LGTM - Updated FastAPI initialization with lifespan parameterProperly updated the FastAPI initialization to include the lifespan parameter, connecting the application lifecycle to database connection management.
48-55:❓ Verification inconclusive
Proper implementation of application lifecycle for database connections
The
lifespanfunction follows the established pattern in the codebase for connecting to MongoDB at application startup and properly disconnecting when the application shuts down. This implementation ensures proper resource management.However, I noticed that similar lifespan implementations in other services (like in
actions/server.pyandapi/app/main.py) also callAccountProcessor.load_system_properties(). Is this intentional for the evaluator service to not load system properties?
🏁 Script executed:
#!/bin/bash # Check if EvaluatorProcessor might need system properties grep -r "AccountProcessor.load_system_properties" --include="*.py" ./kairon/evaluator/ # Check if there are references to system properties in the evaluator module grep -r "system_properties" --include="*.py" ./kairon/evaluator/Length of output: 151
Clarification Needed: Evaluator Service Database Lifecycle and System Properties Initialization
The implementation of the database connection in
kairon/evaluator/main.pycorrectly follows the established pattern for managing MongoDB connections. However, unlike similar lifespan functions inactions/server.pyandapi/app/main.py—which also callAccountProcessor.load_system_properties()—the evaluator service does not load system properties. Our grep commands in the evaluator module returned no references toAccountProcessor.load_system_properties()orsystem_properties.Please confirm whether the evaluator service is intentionally designed to omit system properties initialization. If this is by design, consider adding a comment or documentation to clarify the rationale; otherwise, please update the evaluator's lifecycle to include the necessary system properties loading.
tests/unit_test/callback_test.py (1)
398-402:❓ Verification inconclusive
Verify response extraction logic.
The test assumes that the implementation correctly extracts the
bot_responsekey from the nested body structure in the response. However, it doesn't test scenarios where the response structure might be different (missing keys or different nesting).
🏁 Script executed:
#!/bin/bash # Search for the implementation of run_pyscript to verify how it handles the response structure rg -A 10 "def run_pyscript" --glob "kairon/async_callback/processor.py"Length of output: 1854
Response Extraction Verification:
The current test intests/unit_test/callback_test.pyonly verifies the straightforward case whererun_pyscriptreturns a dictionary with thebot_responsekey (i.e.,{'bot_response': 'Test response'}). However, based on the implementation inkairon/async_callback/processor.py, the extraction logic for processing the lambda response isn’t explicitly shown. This leads to potential gaps in handling variations in the response structure (e.g., missingbot_responsekey or different nesting).
- Action Items:
- Add tests to simulate scenarios with alternative response structures, such as:
- A response missing the
bot_responsekey.- A response where
bot_responseis nested within another key.- An error or unexpected structure returned from the lambda invocation.
- Ensure that
run_pyscriptrobustly extracts the required value or properly handles cases when the expected key is lacking.tests/unit_test/api/api_processor_test.py (2)
62-66: LGTM: Good addition of a fixture to clean up test data.This new
delete_default_accountfixture properly ensures a clean state before running tests by removing specific test user and account entries. This helps prevent test interference and improves test reliability.
1148-1148: LGTM: Good use of the fixture for test setup.You've properly updated the test signature to use the newly created fixture, ensuring the database has a clean state before running this test case.
kairon/async_callback/processor.py (3)
9-9: New import aligns with the updated callback evaluation logic.
This import is consistent with the shift toCallbackUtilityfor pyscript handling.
11-11: Import for callback data objects and execution modes is appropriate.
These are relevant for managing and logging callback events.
191-191: Returning data, message, and error_code is concise and clear.
No issues with this final return statement.tests/integration_test/evaluator_service_test.py (3)
291-291: Addition of predefined_objects with empty slot.
This covers scenarios where minimal context is passed to the script.
332-338: Enhanced script test for unauthorized imports.
Checking againstnumpyensures that disallowed imports are correctly flagged.
354-354: Assertion verifies the correct rejection message for unauthorized import.
This aligns with the updated script logic.tests/integration_test/callback_service_test.py (23)
6-6: Library usage
The new import fromblacksheep.contentsforJSONContentlooks good and aligns with BlackSheep usage.
13-13: Importing app from new BlackSheep-based main
This aligns with the revised async approach.
15-15: Importing process_router_message
No issues found with this addition.
16-16: Importing AppException
No concerns.
213-225: Asynchronous GET callback test
Implementation looks good. Consider also asserting headers or status code details as needed.
230-248: Asynchronous POST callback test
Usage ofJSONContentfor sending the request body is correct.
253-268: Asynchronous POST with non-JSON body
Code seems correct for testing fallback request handling.
272-285: Asynchronous PUT callback test
All checks look good.
289-302: Asynchronous PATCH callback test
Approach validated; no concerns.
306-319: Asynchronous DELETE callback test
Implementation is straightforward and consistent.
327-344: Asynchronous callback flow
Verifying async flow withmock_run_pyscript_asyncis tested thoroughly.
346-364: Pyscript failure scenario
Error handling withAppExceptionis correctly validated.
367-380: Dispatch message failure scenario
Approach is correct for verifying exception handling under dispatch issues.
382-396: GET callback with shortened URL
Logic and assertions appear accurate.
400-415: POST callback with shortened URL
Implementation is correct.
419-437: PUT callback with shortened URL
Test scenario is well-covered.
442-461: POST standalone callback scenario
The test covers relevant data and checks.
466-485: Standalone callback missing identifier path
Error scenario is handled properly.
490-507: Standalone callback with incorrect identifier
No concerns.
511-527: Standalone callback using shortened URL
Test scenario verified.
533-549: Shortened URL with invalid token
Error response is tested thoroughly.
555-580: State change test
Repeated calls demonstrate incrementing state logic.
582-591: Additional state change checks
Implementation is consistent with earlier logic.tests/unit_test/evaluator/evaluator_test.py (3)
30-30: Use of predefined_objects
Addingpredefined_objects={"slot": {}}is consistent with the new evaluation approach.
53-60: Unauthorized numpy import test
The test correctly checks for an unauthorized import scenario and raises anAppException, ensuring security constraints.
68-68: Updated interpreter error test
Extending the test withpredefined_objectsis aligned with the new script evaluation logic.tests/unit_test/callback/pyscript_handler_test.py (1)
1-11: Comprehensive tests forpyscript_handler
These cover valid scripts, unauthorized imports, scheduled jobs, and CRUD operations thoroughly.Also applies to: 13-527, 529-1081
kairon/async_callback/main.py (15)
1-1: Asynchronous import
No specific issues.
5-5: Load environment
Ensures environment variables are loaded before further setup.
9-9: Importing pyscript_callback
Transitioning to the new router is consistent with BlackSheep integration.
11-12: Switch to BlackSheep Application
UsingApplication,Request, andResponsealigns with the new async framework design.
19-28: Startup function
Ensures MongoDB connections are established at application start.
30-36: Shutdown function
Properly handles MongoDB disconnection.
38-40: Registering app events
Attachingstartupandshutdownensures correct lifecycle management for the application.
71-77: Open CORS usage
app.use_cors(allow_origins=["*"], ...)is permissive. If this is production, consider restricting allowed origins for security.
80-91: ** catch_exceptions_middleware**
Returning JSON for unhandled exceptions ensures consistent error responses.
96-112: Security configuration objects
These dictionaries help generate secure headers. The approach is flexible and readable.
115-121: generate_hsts_header
Implementation is straightforward, constructing valid HSTS directives.
124-147: add_security_headers_middleware
Correctly appends security headers to responses.
148-149: Registering middlewares
This finalizes security and exception handling.
152-155: Index endpoint
Returns a straightforward success message for the root route.
157-160: Health check endpoint
Provides a clear "Health check OK" verification.kairon/shared/concurrency/actors/pyscript_runner.py (2)
49-58: Confirm concurrency-safety of partial usage.Attaching
partial(CallbackUtility.*)calls with the samebotreference on a global dictionary can have concurrency implications if multiple scripts run in parallel. Using global references can lead to race conditions or unexpected behavior. Ensure these calls are safe for concurrent execution, or refactor to create these partials in local scope.
20-22:❓ Verification inconclusive
Double-check necessity of newly allowed modules.
You've allowed the
googlemaps,decimal, and_strptimemodules here, which can expand the attack surface if not strictly needed. Please verify you truly require all three to avoid introducing unnecessary complexity or security risks.
🏁 Script executed:
#!/bin/bash # Description: Search the codebase for references to the newly allowed modules to confirm usage rg 'googlemaps|decimal|_strptime' -A 5Length of output: 2240
Review: Confirm Necessity of Allowed Modules
- Both
googlemapsand_strptimeare allowed in multiple files (e.g., in bothkairon/shared/concurrency/actors/pyscript_runner.pyandkairon/async_callback/utils.py), which suggests they are used by different parts of the application.- The
decimalmodule is only explicitly allowed inpyscript_runner.pywithout any additional references elsewhere in the codebase. Please verify ifdecimalis truly needed here. If not, consider removing it to reduce the potential attack surface.kairon/async_callback/utils.py (4)
20-39: Ensure that allowing certain modules in RestrictedPython is necessary.
Grantingrequestsandgooglemapsmay open additional surfaces for unauthorized access in scripts. Confirm this is intended.
40-45: UUID7 usage looks solid.
This approach is well-suited for generating unique and time-sortable IDs.
132-152: Job deletion workflow is well-structured.
The existing approach for cleanup job scheduling looks consistent with your new event server integration.
181-197: Logs might expose sensitive data.
perform_cleanuplogs local variables. Double-check that no personally identifiable or sensitive information is leaked to logs.tests/unit_test/actors/actors_test.py (2)
334-365: Tests for date formatting and parsing are comprehensive.
This showcases a clear demonstration of mocking datetime. Good coverage.
368-475: Vector/search operation tests look solid.
Multiple scenarios (embedding search, payload search, no-op) are covered, ensuring robust coverage of the utility.🧰 Tools
🪛 Ruff (0.8.2)
376-376: Local variable
mock_get_llm_secretis assigned to but never usedRemove assignment to unused variable
mock_get_llm_secret(F841)
377-377: Local variable
mock_litellmis assigned to but never usedRemove assignment to unused variable
mock_litellm(F841)
| mock_environment = {"evaluator": {"pyscript": {"trigger_task": True, 'url': "http://localhost:8080/evaluate"}}, | ||
| "action": {"request_timeout": 1}} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider parametrizing the request timeout value.
The request timeout is hardcoded to 1 second, which might be too short for some test environments, potentially leading to flaky tests. Consider making this configurable or increase to a more reasonable value.
Also applies to: 1442-1443
🏁 Script executed:
#!/bin/bash
# Check if there are any other timeout values used in the codebase for comparison
rg -g "*.py" "request_timeout" --no-headingLength of output: 989
Action Required: Make the request timeout configurable for action tests
The request_timeout parameter for action tasks is hardcoded to 1 second in multiple tests (e.g., in tests/integration_test/action_service_test.py on lines 1366-1367 and 1442-1443, as well as in other test files). Since similar configurations (like those for llm) use a more generous default of 30 seconds, a 1-second timeout might lead to flaky tests in slower or more variable environments. Please consider parameterizing this timeout value—either by accepting it as a configurable parameter (potentially via a configuration file or environment variable) or by increasing the default value to a more robust setting.
|
|
||
| from kairon import Utility | ||
| from fastapi.testclient import TestClient | ||
| from blacksheep.testing import TestClient |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate the unused or conflicting TestClient from FastAPI
You're now using TestClient from blacksheep.testing. If the FastAPI import from line 9 is no longer needed, consider removing it to avoid confusion or conflicts.
🧰 Tools
🪛 Ruff (0.8.2)
10-10: Redefinition of unused TestClient from line 9
(F811)
| method=responses.DELETE, | ||
| url=http_url, | ||
| json={"data": "Deleted Successfully", "message": "OK", "success": True, "error_code": 0}, | ||
| status=200 |
There was a problem hiding this comment.
Potential Generic API Key exposed
Static analysis flagged line 528 as possibly referencing a generic API key. Verify that no real credentials are exposed.
🧰 Tools
🪛 Gitleaks (8.21.2)
528-528: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| bot = predefined_objects.get("slot", {}).get("bot") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential None value for 'bot'.
bot may be None if the predefined_objects["slot"] dictionary does not contain a "bot" key. Consider validating or defaulting this value to avoid downstream errors in partial calls.
| logger.exception(e) | ||
| return BSResponse( | ||
| status=500, | ||
| content=JSONContent({ | ||
| "message": str(e), | ||
| "error_code": 400, | ||
| "data": None, | ||
| "success": False | ||
| }) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify your error code and HTTP status.
When catching unforeseen exceptions, you return a 500 HTTP status but set error_code to 400, which can cause confusion in error handling. Consider aligning them for consistency, for example using 500 for both.
return BSResponse(
status=500,
content=JSONContent({
- "message": str(e),
- "error_code": 400,
+ "message": f"Unexpected server error: {e}",
+ "error_code": 500,
"data": None,
"success": False
})
)📝 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.
| logger.exception(e) | |
| return BSResponse( | |
| status=500, | |
| content=JSONContent({ | |
| "message": str(e), | |
| "error_code": 400, | |
| "data": None, | |
| "success": False | |
| }) | |
| ) | |
| logger.exception(e) | |
| return BSResponse( | |
| status=500, | |
| content=JSONContent({ | |
| "message": f"Unexpected server error: {e}", | |
| "error_code": 500, | |
| "data": None, | |
| "success": False | |
| }) | |
| ) |
| logger.debug(e) | ||
| raise Exception(f"Error converting payload to JSON: {value}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Chain the original exception in raise.
Raising an exception in the except block without chaining can lose context. Use raise Exception(...) from e to preserve the original traceback.
except json.JSONDecodeError as e:
logger.debug(e)
- raise Exception(f"Error converting payload to JSON: {value}")
+ raise Exception(f"Error converting payload to JSON: {value}") 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.
| logger.debug(e) | |
| raise Exception(f"Error converting payload to JSON: {value}") | |
| logger.debug(e) | |
| raise Exception(f"Error converting payload to JSON: {value}") from e |
🧰 Tools
🪛 Ruff (0.8.2)
129-129: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| import pickle | ||
| from calendar import timegm | ||
| from datetime import datetime, date | ||
| from requests import Response | ||
| from functools import partial | ||
| from types import ModuleType | ||
| from typing import Text, Dict, Callable | ||
| import requests | ||
| from AccessControl.SecurityInfo import allow_module | ||
| from apscheduler.triggers.date import DateTrigger | ||
| from apscheduler.util import obj_to_ref, astimezone | ||
| from bson import Binary | ||
| from pymongo import MongoClient | ||
| from RestrictedPython.Guards import safer_getattr | ||
| import orjson as json | ||
| from tzlocal import get_localzone | ||
| from uuid6 import uuid7 | ||
| from loguru import logger | ||
|
|
There was a problem hiding this comment.
Beware of potential security risks when storing pickle data in a database.
Pickle can pose security risks if malicious data is deserialized. Consider using safer serialization (e.g., JSON) if the data is untrusted.
kairon/async_callback/utils.py
Outdated
| @staticmethod | ||
| def add_data(user: str, payload: dict, bot: str = None): | ||
| if not bot: | ||
| raise Exception("Missing bot id") | ||
|
|
||
| collection_id = cognition_processor.save_collection_data(payload, user, bot) | ||
| return { | ||
| "message": "Record saved!", | ||
| "data": {"_id": collection_id} | ||
| } | ||
|
|
||
| @staticmethod | ||
| def update_data(collection_id: str, user: str, payload: dict, bot: str = None): | ||
| if not bot: | ||
| raise Exception("Missing bot id") | ||
|
|
||
| collection_id = cognition_processor.update_collection_data(collection_id, payload, user, bot) | ||
| return { | ||
| "message": "Record updated!", | ||
| "data": {"_id": collection_id} | ||
| } | ||
|
|
||
| @staticmethod | ||
| def delete_data(collection_id: str, user: Text, bot: Text = None): | ||
| if not bot: | ||
| raise Exception("Missing bot id") | ||
|
|
||
| cognition_processor.delete_collection_data(collection_id, bot, user) | ||
|
|
||
| return { | ||
| "message": f"Collection with ID {collection_id} has been successfully deleted.", | ||
| "data": {"_id": collection_id} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency in error handling for database operations.
Similar to get_data, these methods (add_data, update_data, delete_data) raise Exception instead of AppException. Standardize across the codebase for clarity.
| } | ||
| } | ||
| } | ||
| headers = {"D360-API-KEY" : "abcdcffdfdv1tsh9qlr9Oul5AK"} |
There was a problem hiding this comment.
Potentially exposed API key in test code.
Real keys should be stored securely or mocked. Redact or confirm these placeholders are non-sensitive.
Also applies to: 161-161
🧰 Tools
🪛 Gitleaks (8.21.2)
117-117: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/unit_test/callback_test.py (1)
383-403: Consider reordering patch decorators for clarity
The logic appears correct. However, be mindful that the patch for'kairon.async_callback.processor.CallbackUtility'is declared before the patch for'kairon.async_callback.processor.Utility', so the function parameters may appear in reverse order. This can cause confusion. Consider swapping the decorators or adjusting the function signature to better match typical usage.tests/unit_test/callback/pyscript_handler_test.py (2)
7-7: Remove the unused import
DeepDiffis never used in the file. Please remove it to keep the code clean and avoid confusion.- from deepdiff import DeepDiff🧰 Tools
🪛 Ruff (0.8.2)
7-7:
deepdiff.DeepDiffimported but unusedRemove unused import:
deepdiff.DeepDiff(F401)
12-12: Use uppercase environment variable name
Consider renaming"system_file"to"SYSTEM_FILE"for consistency with environment variable naming conventions, if desired.- os.environ["system_file"] = "./tests/testing_data/system.yaml" + os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.2)
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
tests/unit_test/actors/actors_test.py (3)
8-8: Remove possibly unused import
litellmis imported but primarily patched via string references. If it's not directly called, consider removing it.- import litellm🧰 Tools
🪛 Ruff (0.8.2)
8-8:
litellmimported but unusedRemove unused import:
litellm(F401)
14-15: Unused imports
HttpActionConfigandActionUtilityappear unused here. You can remove them to eliminate lint warnings.- from kairon.shared.actions.data_objects import DatabaseAction, HttpActionConfig - from kairon.shared.actions.utils import ActionUtility + from kairon.shared.actions.data_objects import DatabaseAction🧰 Tools
🪛 Ruff (0.8.2)
14-14:
kairon.shared.actions.data_objects.HttpActionConfigimported but unusedRemove unused import:
kairon.shared.actions.data_objects.HttpActionConfig(F401)
15-15:
kairon.shared.actions.utils.ActionUtilityimported but unusedRemove unused import:
kairon.shared.actions.utils.ActionUtility(F401)
369-388: test_get_embedding
Mocks embedding calls and checks the result. If you don’t need to referencemock_litellm, consider removingas mock_litellmto avoid unused variable warnings.🧰 Tools
🪛 Ruff (0.8.2)
378-378: Local variable
mock_get_llm_secretis assigned to but never usedRemove assignment to unused variable
mock_get_llm_secret(F841)
379-379: Local variable
mock_litellmis assigned to but never usedRemove assignment to unused variable
mock_litellm(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/unit_test/actors/actors_test.py(6 hunks)tests/unit_test/callback/pyscript_handler_test.py(1 hunks)tests/unit_test/callback_test.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unit_test/callback/pyscript_handler_test.py (1)
kairon/async_callback/processor.py (1)
async_callback(85-122)
🪛 Ruff (0.8.2)
tests/unit_test/callback/pyscript_handler_test.py
7-7: deepdiff.DeepDiff imported but unused
Remove unused import: deepdiff.DeepDiff
(F401)
12-12: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
tests/unit_test/actors/actors_test.py
8-8: litellm imported but unused
Remove unused import: litellm
(F401)
14-14: kairon.shared.actions.data_objects.HttpActionConfig imported but unused
Remove unused import: kairon.shared.actions.data_objects.HttpActionConfig
(F401)
15-15: kairon.shared.actions.utils.ActionUtility imported but unused
Remove unused import: kairon.shared.actions.utils.ActionUtility
(F401)
72-72: Redefinition of unused HttpActionConfig from line 14
Remove definition: HttpActionConfig
(F811)
378-378: Local variable mock_get_llm_secret is assigned to but never used
Remove assignment to unused variable mock_get_llm_secret
(F841)
379-379: Local variable mock_litellm is assigned to but never used
Remove assignment to unused variable mock_litellm
(F841)
491-491: Local variable mock_get_payload is assigned to but never used
Remove assignment to unused variable mock_get_payload
(F841)
🪛 Gitleaks (8.21.2)
tests/unit_test/callback/pyscript_handler_test.py
365-365: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/unit_test/actors/actors_test.py
119-119: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
163-163: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (37)
tests/unit_test/callback/pyscript_handler_test.py (24)
19-65: Validation of simple pyscript execution
The test correctly verifies execution of a basic script and checks the returned bot response. The coverage is good, and no issues are found.
67-83: Validates script without predefined objects
All assertions pass, and the test helps ensure the code handles an empty predefined context gracefully.
85-109: Appropriate handling of unauthorized imports
The test confirms that importing disallowed libraries (e.g.,numpy) is blocked, fulfilling security and sandbox constraints.
111-141: List-based event data
Passing event data as a list is properly handled. The script runs, and the final assertion ensures the response is as expected.
144-180: Handling datetime objects in event data
The test verifies that datetime objects in input data are correctly converted and included in the output. Looks good.
182-218: Handling date objects in event data
Likewise, the test checks correct formatting ofdateobjects, ensuring predictable date string conversion.
220-262: Handling Response objects in event data
This ensures responses from libraries likerequestsare safely stringified and included. The test is well-structured.
264-315: Test sending email with valid bot
The script triggers an email action and confirms a success message. No issues found.
317-346: Test sending email without bot
Checks that missing bot context yields an appropriate error. Validation logic is correct.
348-440: Scheduled job addition flow
The code tests adding scheduled jobs in the script, verifying the final response. Implementation appears consistent.🧰 Tools
🪛 Gitleaks (8.21.2)
365-365: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
442-506: Scheduled job addition without bot
Properly handles the missing bot scenario and returns the expected error. Looks good.
508-543: Deleting a scheduled job without bot
Ensures the error message is returned if bot ID is missing. No concerns here.
545-580: Deleting a scheduled job without event ID
Similar validation for missing event ID. The test is well-structured.
582-624: Deleting a scheduled job
Checks successful deletion scenario and final response correctness. Implementation is solid.
626-676: CRUD - Add data
Thoroughly tests adding data to a collection and verifying the response. No issues.
678-716: Add data without bot
Covers the missing bot error path thoroughly. Good negative test coverage.
718-771: CRUD - Get data
Usespyscript_handlerto retrieve data. Assertions confirm the correct keys and structure.
773-811: Get data without bot
Again, ensures graceful handling when the bot ID is absent.
813-857: CRUD - Update data
Validates updates to an existing record. The test flow is coherent.
859-899: Update data without bot
Properly tests missing bot condition and verifies the error.
901-945: Get updated data
Confirms the record has been updated successfully. The test is robust.
947-981: CRUD - Delete data
Ensures a record can be removed and verifies the success message.
983-1023: Delete data without bot
Good negative scenario coverage for the delete operation.
1025-1076: Get data after delete
Ensures that the record is absent after deletion. The test is thorough.tests/unit_test/actors/actors_test.py (13)
4-6: New imports
•datetime,patch,MagicMock, andurljoinare introduced. They seem appropriately used below.
72-72: Potential generic API key exposure
Lines 72, 119, and 163 reference"HttpActionConfig"redefinition or a literal string for"D360-API-KEY". If this key is a placeholder, confirm it's not sensitive. Otherwise, remove or mask it.Also applies to: 119-119, 163-163
🧰 Tools
🪛 Ruff (0.8.2)
72-72: Redefinition of unused
HttpActionConfigfrom line 14Remove definition:
HttpActionConfig(F811)
181-227: Database action retrieval
The test checksget_db_action_datausage with a mock response. No issues found.
258-269: Script error scenario
Test confirms that an unauthorized import (numpy) leads to an error. Implementation is correct.
287-291: Interpreter error scenario
Ensures syntax errors in the script raise an exception. Logic is correct.
336-346: test_srtptime
Validates date parsing. No issues found.
348-356: test_srtftime
Checks formatting of adatetimeobject. Straightforward and correct.
358-367: test_url_parse_quote_plus
Ensures encoding of strings for URL usage. Implementation is valid.
390-415: test_perform_operation
Assembles embedding and payload search, then posts to the vector DB. The logic seems correct.
417-444: test_perform_operation_embedding_search
Similar to above, but explicitly verifies embedding-based searching with a user-provided vector DB URL.
445-468: test_perform_operation_payload_search
Checks payload-based search without embedding. Properly forms the request to the vector DB.
469-477: test_perform_operation_no_operation
Correctly raises an exception if neitherembedding_searchnorpayload_searchis present.
478-508: test_get_db_action_data
UsesDatabaseActionand calls internal utilities for constructing queries. Ifmock_get_payloadisn’t needed, remove it to avoid unused variable warnings. Overall logic is good.🧰 Tools
🪛 Ruff (0.8.2)
491-491: Local variable
mock_get_payloadis assigned to but never usedRemove assignment to unused variable
mock_get_payload(F841)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/integration_test/callback_service_test.py (1)
12-13: 🛠️ Refactor suggestionRemove redundant TestClient import.
You're importing
TestClientfrom two different modules. Since you're now using the one from BlackSheep, the FastAPI import should be removed to avoid conflicts.-from fastapi.testclient import TestClient🧰 Tools
🪛 Ruff (0.8.2)
13-13: Redefinition of unused
TestClientfrom line 12(F811)
🧹 Nitpick comments (5)
tests/unit_test/callback/pyscript_handler_test.py (3)
5-5: Remove unused import.The
MagicMockis imported but never used in this file.-from unittest.mock import patch, MagicMock +from unittest.mock import patch🧰 Tools
🪛 Ruff (0.8.2)
5-5:
unittest.mock.MagicMockimported but unusedRemove unused import:
unittest.mock.MagicMock(F401)
9-9: Remove unused import.The
DeepDiffis imported but never used in this file.-from deepdiff import DeepDiff🧰 Tools
🪛 Ruff (0.8.2)
9-9:
deepdiff.DeepDiffimported but unusedRemove unused import:
deepdiff.DeepDiff(F401)
14-14: Use capitalized environment variable naming.Environment variables should be capitalized by convention.
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
kairon/async_callback/utils.py (2)
68-68: Use consistent logging
Rather than usingprint(...), prefer leveraginglogger.info(...)or another appropriate log level. This ensures log messages remain consistent and are captured properly within your logging/monitoring systems.
230-230: Use consistent logging
Like line 68, please replace thisprint(...)statement with a logging call (e.g.,logger.debug(...)orlogger.info(...)) for better traceability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
kairon/async_callback/utils.py(1 hunks)tests/integration_test/callback_service_test.py(5 hunks)tests/unit_test/callback/pyscript_handler_test.py(1 hunks)tests/unit_test/callback_test.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
tests/unit_test/callback/pyscript_handler_test.py (1)
kairon/async_callback/utils.py (4)
CallbackUtility(40-318)pyscript_handler(229-252)generate_id(43-44)datetime_to_utc_timestamp(47-56)
tests/unit_test/callback_test.py (1)
kairon/async_callback/processor.py (3)
CallbackProcessor(19-191)run_pyscript(21-52)async_callback(85-122)
tests/integration_test/callback_service_test.py (2)
kairon/async_callback/processor.py (1)
async_callback(85-122)kairon/async_callback/router/pyscript_callback.py (1)
process_router_message(12-82)
🪛 Ruff (0.8.2)
tests/unit_test/callback/pyscript_handler_test.py
5-5: unittest.mock.MagicMock imported but unused
Remove unused import: unittest.mock.MagicMock
(F401)
9-9: deepdiff.DeepDiff imported but unused
Remove unused import: deepdiff.DeepDiff
(F401)
14-14: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
tests/integration_test/callback_service_test.py
13-13: Redefinition of unused TestClient from line 12
(F811)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (15)
tests/unit_test/callback/pyscript_handler_test.py (2)
518-518: Potential Generic API Key exposed.Static analysis flagged this line as possibly referencing a generic API key. Consider using a mock value instead of a potential real credential, even in tests.
18-1196: Test suite looks comprehensive and well-structured.The test suite effectively covers various scenarios for the
pyscript_handlerfunction:
- Simple script execution
- Invalid imports
- Different types of event data
- CRUD operations
- Email sending
- Scheduled jobs
- Error handling
The assertion patterns are consistent and validate both the status codes and response bodies.
tests/integration_test/callback_service_test.py (4)
186-196: Good implementation of asynchronous root endpoint test.This test correctly verifies the root endpoint of the BlackSheep server, ensuring it returns the expected message and success status.
199-210: Good implementation of asynchronous health check test.This test properly verifies the health check endpoint functionality, asserting on both the response status and message.
330-344: Well-implemented fallback test for text content.This test appropriately handles the case where JSON decoding fails and the request falls back to reading text content, which is an important edge case to cover.
573-608: Great state management testing.This test thoroughly verifies that state is maintained between multiple calls to the same endpoint, testing an important aspect of the stateful callback functionality.
tests/unit_test/callback_test.py (5)
384-402: Good implementation of run_pyscript test with CallbackUtility.The test successfully validates the new implementation that uses
CallbackUtility.pyscript_handlerinstead of the previous cloud-based solution. It properly mocks the dependencies and checks the expected output.
404-427: Good test coverage for lambda execution path.This test successfully validates the lambda execution path with proper mocking and verification of the expected function calls.
429-450: Good error handling test for lambda execution failure.This test appropriately verifies that when the lambda execution returns an error message, an exception is properly raised and propagated.
452-466: Good server failure test.This test verifies that when the pyscript handler returns a non-200 status code, an appropriate exception is raised with the error message from the response body.
560-600: Good edge case coverage for empty responses.These two tests properly validate behavior when empty or null responses are received, ensuring that appropriate error messages are generated and the failure entry is created.
kairon/async_callback/utils.py (4)
117-121: Potential security risk with pickled job state
This replicates a previously identified concern: deserializing pickle data from a database can pose a serious security risk if content is untrusted. Consider safer serialization formats such as JSON and storing only necessary data to mitigate potential exploits.
198-227: Good use of partial with restricted environment
Allowing only predefined objects and usingRestrictedPythonensures robust isolation. This design effectively conveys minimal privileges to script executions. Great work here—no major concerns.
228-253: Validate event data and handle missing keys
If the incomingeventlacks asource_codekey, line 244 would fail when accessingdata['source_code']. Ensure upstream sources guarantee the key, or add checks here to handle such an omission gracefully.
275-275: Inconsistent usage ofException
These lines raise the built-inExceptionwhen the bot ID is missing, whereas other parts of the file rely onAppException. This is the same concern identified previously about standardizing error handling across all data-related methods.Also applies to: 288-288, 299-299, 310-310
| def add_schedule_job(schedule_action: Text, date_time: datetime, data: Dict, timezone: Text, _id: Text = None, | ||
| bot: Text = None, kwargs=None): | ||
|
|
||
| if not bot: | ||
| raise AppException("Missing bot id") | ||
|
|
||
| if not _id: | ||
| _id = uuid7().hex | ||
|
|
||
| print(f"bot: {bot}, name: {schedule_action}") | ||
|
|
||
| if not data: | ||
| data = {} | ||
|
|
||
| data['bot'] = bot | ||
| data['event'] = _id | ||
|
|
||
| callback_config = CallbackConfig.get_entry(bot=bot, name=schedule_action) | ||
|
|
||
| script = callback_config.get('pyscript_code') | ||
|
|
||
| func = obj_to_ref(ExecutorFactory.get_executor().execute_task) | ||
|
|
||
| schedule_data = { | ||
| 'source_code': script, | ||
| 'predefined_objects': data | ||
| } | ||
|
|
||
| args = (func, "scheduler_evaluator", schedule_data,) | ||
| kwargs = {'task_type': "Callback"} if kwargs is None else {**kwargs, 'task_type': "Callback"} | ||
| trigger = DateTrigger(run_date=date_time, timezone=timezone) | ||
|
|
||
| next_run_time = trigger.get_next_fire_time(None, datetime.now(astimezone(timezone) or get_localzone())) | ||
|
|
||
| job_kwargs = { | ||
| 'version': 1, | ||
| 'trigger': trigger, | ||
| 'executor': "default", | ||
| 'func': func, | ||
| 'args': tuple(args) if args is not None else (), | ||
| 'kwargs': kwargs, | ||
| 'id': _id, | ||
| 'name': "execute_task", | ||
| 'misfire_grace_time': 7200, | ||
| 'coalesce': True, | ||
| 'next_run_time': next_run_time, | ||
| 'max_instances': 1, | ||
| } | ||
|
|
||
| logger.info(job_kwargs) | ||
|
|
||
| client = MongoClient(Utility.environment['database']['url']) | ||
| events_db_name = Utility.environment["events"]["queue"]["name"] | ||
| events_db = client.get_database(events_db_name) | ||
| scheduler_collection = Utility.environment["events"]["scheduler"]["collection"] | ||
| job_store_name = events_db.get_collection(scheduler_collection) | ||
| event_server = Utility.environment['events']['server_url'] | ||
|
|
||
| job_store_name.insert_one({ | ||
| '_id': _id, | ||
| 'next_run_time': CallbackUtility.datetime_to_utc_timestamp(next_run_time), | ||
| 'job_state': Binary(pickle.dumps(job_kwargs, pickle.HIGHEST_PROTOCOL)) | ||
| }) | ||
|
|
||
| http_response = ActionUtility.execute_http_request( | ||
| f"{event_server}/api/events/dispatch/{_id}", | ||
| "GET") | ||
|
|
||
| if not http_response.get("success"): | ||
| raise AppException(http_response) | ||
| else: | ||
| logger.info(http_response) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine scheduling logic & unify exception usage
Within add_schedule_job, you correctly raise AppException for missing fields (line 63), but other methods in this file rely on the built-in Exception. Consider unifying around AppException for consistent error handling. Additionally, this method performs multiple responsibilities (e.g., generating job arguments, persisting job state in MongoDB, invoking an external event server). Splitting out scheduling logic into a helper function or class would improve maintainability. Also note that rebuilding kwargs on line 88 could overshadow existing keys—renaming variables or merging maps more explicitly can help clarify usage.
Co-authored-by: spandan_mondal <spandan.mondal@nimblework.com>
* flow tag in import and export * flow tag in import and export * flow tag in import and export * flow tag in import and export * flow tag in import and export --------- Co-authored-by: spandan_mondal <spandan.mondal@nimblework.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
kairon/importer/validator/file_validator.py (1)
1115-1130: New static method for rules validationThis new method validates rules defined in a YAML file against a schema, which enhances the validation capabilities of the system.
The implementation is well-structured:
- Locates the schema file relative to the current file
- Loads both schema and rules from their respective files
- Performs validation and raises appropriate errors
Consider enhancing the error message to provide more specific details about which rules failed validation and why.
- if not validator.validate(rules): - raise ValueError(f"Validation errors: {validator.errors}") + if not validator.validate(rules): + error_details = '\n'.join([f"- {k}: {v}" for k, v in validator.errors.items()]) + raise ValueError(f"Rules validation failed with the following errors:\n{error_details}")kairon/shared/data/model_data_imporer.py (2)
78-81: Consider using a logging mechanism instead of print statements.While printing flow tags may be useful for debugging, relying on
- print("CustomStoryGraph created with flow_tags:", self.flow_tags) + import logging + logger = logging.getLogger(__name__) + logger.info(f"CustomStoryGraph created with flow_tags: {self.flow_tags}")
102-116: Remove.keys()when checking membership in dictionaries.To improve readability and comply with static analysis suggestions (Ruff SIM118), remove redundant
.keys()calls. Instead ofif key in dict.keys(), you can directly useif key in dict.- elif KEY_USER_INTENT in step.keys() or KEY_USER_MESSAGE in step.keys(): + elif KEY_USER_INTENT in step or KEY_USER_MESSAGE in step: - elif KEY_OR in step.keys(): + elif KEY_OR in step: - elif KEY_ACTION in step.keys(): + elif KEY_ACTION in step: - elif KEY_BOT_END_TO_END_MESSAGE in step.keys(): + elif KEY_BOT_END_TO_END_MESSAGE in step: - elif KEY_CHECKPOINT in step.keys(): + elif KEY_CHECKPOINT in step: - elif KEY_SLOT_NAME in step.keys(): + elif KEY_SLOT_NAME in step: - elif KEY_ACTIVE_LOOP in step.keys(): + elif KEY_ACTIVE_LOOP in step: - elif KEY_METADATA in step.keys(): + elif KEY_METADATA in step:🧰 Tools
🪛 Ruff (0.8.2)
102-102: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
102-102: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
104-104: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
106-106: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
108-108: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
110-110: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
112-112: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
114-114: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
116-116: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
kairon/shared/data/processor.py (2)
4989-4995: Optimize multiple isinstance checksThe code now checks for both
CustomRuleStepandRuleStepinstances, but uses multipleisinstancecalls which can be optimized.Use a tuple of types with a single
isinstancecall for better performance:- if ( - (isinstance(story_step, CustomRuleStep) or isinstance(story_step, RuleStep)) - and story_step.block_name.strip().lower() not in saved_rules - ): + if ( + isinstance(story_step, (CustomRuleStep, RuleStep)) + and story_step.block_name.strip().lower() not in saved_rules + ):🧰 Tools
🪛 Ruff (0.8.2)
4994-4994: Multiple
isinstancecalls forstory_step, merge into a single callMerge
isinstancecalls forstory_step(SIM101)
4998-5000: Ensure proper type annotation for the method parameterThe method parameter type annotation should be updated to match the implementation.
Update the type annotation to reflect that the method accepts either
CustomRuleSteporRuleStep:-def __extract_rule_events(self, rule_step: CustomRuleStep | RuleStep, bot: str, user: str): +def __extract_rule_events(self, rule_step: CustomRuleStep | RuleStep, bot: str, user: str):Note: The type annotation already correctly uses the pipe operator (
|) for union types, which is the preferred syntax in Python 3.10+.tests/unit_test/validator/data_importer_test.py (3)
328-336: Missing assertions in test_krasa_file_importerThis test is only verifying that
get_stories()doesn't raise exceptions, but it doesn't actually assert that the method behaves correctly or returns the expected data.Consider adding assertions to verify the expected behavior, e.g.:
def test_krasa_file_importer(monkeypatch): importer = KRasaFileImporter() importer._story_files = ["test.yml"] - importer.get_domain = Mock() + importer.get_domain = Mock(return_value=Mock()) dummy_yaml = "stories:\n- story: test\n steps: []" monkeypatch.setattr("rasa.shared.utils.io.read_file", lambda filename, encoding='utf-8': dummy_yaml) monkeypatch.setattr("rasa.shared.utils.validation.validate_yaml_schema", lambda s, f: None) - importer.get_stories() + stories = importer.get_stories() + assert isinstance(stories, CustomStoryGraph) + assert len(stories.story_steps) >= 0
437-472: Consider explaining the test expectations more clearlyThe test is thorough in mocking the parser behavior, but it would benefit from a clearer explanation of why we expect to see the dummy step twice in the result.
Consider adding a comment to explain the expected behavior:
steps = reader.read_from_parsed_yaml(parsed_content) +# We expect to see dummy_step twice because we're parsing both stories and rules +# and the fake parser returns the same dummy_step for both assert steps.count(dummy_step) == 2
491-515: Add explanation for magic number in exclusion percentageThe test uses 50 as the exclusion percentage without explaining the significance of this value.
Consider adding a comment to explain the choice of exclusion percentage:
- steps = KRasaFileImporter.load_data_from_files(story_files, dummy_domain, exclusion_percentage=50) + # Using 50% exclusion should result in keeping all 10 steps in this test scenario + # In a real scenario, it would exclude approximately half of the steps + steps = KRasaFileImporter.load_data_from_files(story_files, dummy_domain, exclusion_percentage=50)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
kairon/importer/validator/file_validator.py(4 hunks)kairon/shared/data/model_data_imporer.py(1 hunks)kairon/shared/data/processor.py(8 hunks)kairon/shared/utils.py(3 hunks)system.yaml(1 hunks)tests/unit_test/channels/mail_channel_test.py(0 hunks)tests/unit_test/data_processor/data_processor_test.py(2 hunks)tests/unit_test/events/events_test.py(2 hunks)tests/unit_test/validator/data_importer_test.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/unit_test/channels/mail_channel_test.py
🧰 Additional context used
🧬 Code Definitions (2)
tests/unit_test/events/events_test.py (1)
kairon/shared/data/model_data_imporer.py (1)
KRasaFileImporter(159-183)
kairon/shared/data/processor.py (3)
kairon/shared/data/model_data_imporer.py (2)
KRasaFileImporter(159-183)CustomRuleStep(27-30)kairon/shared/models.py (1)
FlowTagType(127-129)kairon/shared/data/data_objects.py (1)
Rules(716-748)
🪛 Ruff (0.8.2)
kairon/shared/data/model_data_imporer.py
102-102: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
102-102: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
104-104: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
106-106: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
108-108: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
110-110: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
112-112: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
114-114: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
116-116: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
kairon/shared/data/processor.py
4994-4994: Multiple isinstance calls for story_step, merge into a single call
Merge isinstance calls for story_step
(SIM101)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (18)
system.yaml (1)
144-144: New Agentic Flow Task Definition AdditionThe new key
agentic_flow: ${AGENTIC_FLOW_TASK_DEFINITION}has been added under theevents.task_definitionsection. Please ensure that the environment variableAGENTIC_FLOW_TASK_DEFINITIONis properly defined in your deployment environment and that it adheres to the same expected format as the other task definitions. This will help avoid potential runtime misconfigurations.tests/unit_test/data_processor/data_processor_test.py (2)
5281-5281: LGTM: Updated test assertion to include metadata fieldThe test assertion has been correctly updated to include the new
metadatafield withflow_tagsset tochatbot_flow, which aligns with the framework changes in this PR.Is it intentional that only the first rule contains the metadata field? If all rules should have this metadata, make sure your implementation is consistent.
5296-5296: Ensure consistent metadata structure across rulesThe test assertion has been updated to include the
metadatafield withflow_tagsfor the first rule, but I notice the second rulemultiflow_story_story_download_data_files_1doesn't have the equivalent metadata field.Is it intentional that only the first rule contains the metadata field? If this is by design, the test is correct. If all rules should have this metadata, make sure to update the second rule's assertion as well.
tests/unit_test/events/events_test.py (2)
23-23: Import looks good.The import of
KRasaFileImporterfrom the custom Kairon module is correctly added to replace the standardRasaFileImporter.
76-78: Appropriate replacement of importer implementation.The change from
RasaFileImportertoKRasaFileImporteris appropriate here. SinceKRasaFileImporterinherits fromRasaFileImporter(as shown in the relevant code snippets), it should maintain backward compatibility while providing enhanced functionality for story graph processing.kairon/shared/utils.py (3)
86-86: Clean import added for KYAMLStoryWriterThis import adds the new KYAMLStoryWriter class from the model_data_imporer module, which is required for the modification in the write_training_data method.
1206-1206: Updated to use KYAMLStoryWriter instead of YAMLStoryWriterThis change replaces the standard Rasa YAMLStoryWriter with a custom Kairon implementation (KYAMLStoryWriter) for dumping rules, which aligns with the framework migration described in the PR objectives.
1635-1635: Improved type annotations for register_telegram_webhook methodAdding explicit type annotations (
str) for the parameters enhances code readability and enables better type checking, which is a good practice for improving code maintainability.kairon/importer/validator/file_validator.py (3)
18-18: Import from ruamel.yaml addedThe yaml import has been changed to be from ruamel package, which is necessary for the new validate_rules method.
30-30: Replaced import with KRasaFileImporterThis change imports the custom Kairon implementation of RasaFileImporter, which aligns with the framework migration mentioned in the PR objectives.
81-81: Updated to use KRasaFileImporterThe standard RasaFileImporter has been replaced with KRasaFileImporter for importing training data, which is part of the framework migration described in the PR.
kairon/shared/data/processor.py (5)
164-164: Updated import to use custom file importerThe import has been changed from using the standard
RasaFileImporterto using a customKRasaFileImporteralong withCustomRuleStep, which extends RuleStep to support flow tags.
233-233: Changed method call to use new download-specific rule retrievalThe method call has been updated to use
get_rules_for_downloadinstead ofget_rules_for_training, which is a newly implemented method that returns rules with flow tags for download purposes.
508-508: Updated file importer to use custom implementationChanged from using
RasaFileImportertoKRasaFileImporter, which is consistent with the import change and extends the base importer with custom functionality.
5001-5027: Enhanced rule event extraction with flow tags supportThe
__extract_rule_eventsmethod has been updated to handle bothCustomRuleStepandRuleSteptypes, and now extracts flow tags from the rule step if available. This implementation properly initializes default flow tags and condition events indices with appropriate checks.
5092-5118: Added new method for preparing rules with flow tags for downloadThis new method
get_rules_for_downloadcreates aStoryGraphwithCustomRuleStepinstances that include flow tags. It differs fromget_rules_for_trainingby including all rules (not filtering by flow tags) and preserving flow tag information in the returned story graph.tests/unit_test/validator/data_importer_test.py (2)
420-435: Good testing of error conditionThis test effectively captures and verifies warning messages for unexpected step strings. It demonstrates good use of monkeypatching to capture the warning message.
283-286: Well-structured tests with clear assertionsThe test suite is well-organized with focused test functions that make good use of mocking to isolate tests from external dependencies. Each test verifies a specific aspect of the code's behavior with clear assertions.
Also applies to: 288-294, 296-299, 302-316, 319-326, 338-347, 348-351, 355-357, 360-391, 394-417, 476-487
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/unit_test/callback/pyscript_handler_test.py (4)
9-9: Remove unused import to clean up the code.The import
deepdiff.DeepDiffat line 9 is not referenced anywhere in this file. Please remove it to simplify dependencies and eliminate lint warnings.- from deepdiff import DeepDiff🧰 Tools
🪛 Ruff (0.8.2)
9-9:
deepdiff.DeepDiffimported but unusedRemove unused import:
deepdiff.DeepDiff(F401)
16-16: Follow naming conventions for environment variables.Consider renaming
system_filetoSYSTEM_FILEin order to follow the standard practice of using 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)
16-16: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
53-53: Avoid usingprint(data)in test code.Multiple lines contain
print(data)calls, which can clutter test output. Consider using a logging framework or removing these statements, especially if they are only used for debugging.Also applies to: 78-78, 104-104, 130-130, 167-167, 205-205, 248-248, 303-303, 426-426, 502-502, 538-538, 576-576, 612-612, 660-660, 712-712, 752-752, 807-807, 895-895, 943-943, 1019-1019, 1059-1059
1128-1128: Remove or utilize the unused variable.Here,
user = "user1"is never used, which can confuse future maintainers. Either remove it or leverage it if needed.- user = "user1"🧰 Tools
🪛 Ruff (0.8.2)
1128-1128: Local variable
useris assigned to but never usedRemove assignment to unused variable
user(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit_test/callback/pyscript_handler_test.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unit_test/callback/pyscript_handler_test.py (2)
kairon/async_callback/processor.py (1)
async_callback(85-122)kairon/async_callback/utils.py (10)
CallbackUtility(40-318)pyscript_handler(229-252)generate_id(43-44)datetime_to_utc_timestamp(47-56)get_data(274-284)add_data(287-295)update_data(298-306)delete_data(309-318)perform_cleanup(182-196)send_email(154-179)
🪛 Ruff (0.8.2)
tests/unit_test/callback/pyscript_handler_test.py
9-9: deepdiff.DeepDiff imported but unused
Remove unused import: deepdiff.DeepDiff
(F401)
16-16: Use capitalized environment variable SYSTEM_FILE instead of system_file
Replace system_file with SYSTEM_FILE
(SIM112)
1128-1128: Local variable user is assigned to but never used
Remove assignment to unused variable user
(F841)
🪛 Gitleaks (8.21.2)
tests/unit_test/callback/pyscript_handler_test.py
369-369: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
tests/unit_test/callback/pyscript_handler_test.py (2)
369-369: Verify that no sensitive credentials are being exposed.Static analysis flagged the
token_hash(and potentiallytoken_value) as a generic API key. Verify that this is a dummy placeholder and not an actual secret. If possible, store sensitive values in environment variables or a safe configuration store.- token_hash="0191703078f87a039906afc0a219dd5c", - token_value="gAAAAABmxKl5tT0UKwkqYi2n9yV1lFAAJKsZEM0G9w7kmN8NIYR9JKF1F9ecZoUY6P9kClUC_QnLXXGLa3T4Xugdry84ioaDtGF9laXcQl_82Fvs9KmKX8xfa4-rJs1cto1Jd6fqeGIT7mR3kn56_EliP83aGoCl_sk9B0-2gPDgt-EJZQ20l-3OaT-rhFoFanjKvRiE8e4xp9sdxxjgDWLbCF3kCtTqTtg6Wovw3mXZoVzxzNEUmd2OGZiO6IsIJJaU202w3CZ2rPnmK8I2aRGg8tMi_-ObOg==" + import os + token_hash = os.getenv("TEST_TOKEN_HASH", "dummy_token_hash") + token_value = os.getenv("TEST_TOKEN_VALUE", "dummy_token_value")🧰 Tools
🪛 Gitleaks (8.21.2)
369-369: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1-1262: Overall review.These tests comprehensively validate the
pyscript_handlerfunction and related functionalities, covering normal and edge cases effectively. Address the noted issues to further improve maintainability and eliminate potential security concerns.🧰 Tools
🪛 Ruff (0.8.2)
9-9:
deepdiff.DeepDiffimported but unusedRemove unused import:
deepdiff.DeepDiff(F401)
16-16: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
1128-1128: Local variable
useris assigned to but never usedRemove assignment to unused variable
user(F841)
🪛 Gitleaks (8.21.2)
369-369: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Summary by CodeRabbit
New Features
Dependencies
Tests