100% test coverage - #170
Open
shay-te wants to merge 17 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
📝 WalkthroughWalkthroughThis PR adds local Sonar tooling, hardens concurrency and validation behavior across core modules, updates cache, connection, scheduler, session, observer, and web helper flows, and introduces broad unit, regression, and Hypothesis-based property tests. ChangesCore hardening and coverage expansion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)core_lib/cache/cache_handler_redis.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path core_lib/cache/cache_handler.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.28][ERROR]: unable to find a config; path core_lib/cache/cache_handler_memcached.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shay-te. The following files were modified: * `core_lib/alembic/alembic.py` * `core_lib/cache/cache_handler.py` * `core_lib/cache/cache_handler_memcached.py` * `core_lib/cache/cache_handler_no_cache.py` * `core_lib/cache/cache_handler_ram.py` * `core_lib/cache/cache_handler_redis.py` * `core_lib/connection/connection_factory_registry.py` * `core_lib/connection/sql_alchemy_connection.py` * `core_lib/connection/sql_alchemy_connection_factory.py` * `core_lib/core_lib.py` * `core_lib/core_lib_main.py` * `core_lib/data_layers/data/db/sqlalchemy/types/int_enum.py` * `core_lib/data_layers/data/db/sqlalchemy/types/point.py` * `core_lib/data_layers/data_access/db/crud/crud.py` * `core_lib/data_layers/data_access/db/crud/crud_data_access.py` * `core_lib/data_layers/data_access/db/crud/crud_soft_data_access.py` * `core_lib/data_layers/data_access/db/crud/crud_soft_delete_token_data_access.py` * `core_lib/data_transform/helpers.py` * `core_lib/error_handling/duplicate_error_decorator.py` * `core_lib/error_handling/not_found_decorator.py` * `core_lib/helpers/config_instances.py` * `core_lib/helpers/files.py` * `core_lib/helpers/func_utils.py` * `core_lib/helpers/generate_data.py` * `core_lib/helpers/parse_utils.py` * `core_lib/helpers/shell_utils.py` * `core_lib/jobs/job_scheduler.py` * `core_lib/middleware/middleware_chain.py` * `core_lib/observer/observer.py` * `core_lib/observer/observer_decorator.py` * `core_lib/registry/default_registry.py` * `core_lib/rule_validator/helpers.py` * `core_lib/rule_validator/rule_validator.py` * `core_lib/session/jwt_token_handler.py` * `core_lib/session/user_security.py` * `core_lib/web_helpers/decorators.py` * `core_lib/web_helpers/request_response_helpers.py` * `core_lib/web_helpers/require_login_helper.py` * `tests/hypothesis_tests/test_cache_decorator_properties.py` * `tests/hypothesis_tests/test_cache_handler_ram_properties.py` * `tests/hypothesis_tests/test_default_registry_properties.py` * `tests/hypothesis_tests/test_float_to_str_properties.py` * `tests/hypothesis_tests/test_func_utils_properties.py` * `tests/hypothesis_tests/test_generate_datetime_properties.py` * `tests/hypothesis_tests/test_generate_email_properties.py` * `tests/hypothesis_tests/test_middleware_chain_properties.py` * `tests/hypothesis_tests/test_observer_properties.py` * `tests/hypothesis_tests/test_rule_validator_properties.py` * `tests/test_alembic.py` * `tests/test_auth_middlewares.py` * `tests/test_cache_decorator_flags.py` * `tests/test_cache_decorator_stress.py` * `tests/test_cache_handler_ram_full.py` * `tests/test_connections_full.py` * `tests/test_misc_coverage.py` * `tests/test_registry_factory_full.py` * `tests/test_regressions.py` * `tests/test_remaining_gaps.py` * `tests/test_result_to_dict_stress.py` * `tests/test_rule_validator_challenge.py` * `tests/test_shell_utils.py` These files were kept as they were: * `core_lib/data_layers/data/db/join_config/apply_join_configs.py` * `tests/hypothesis_tests/test_any_to_pascal_properties.py` * `tests/hypothesis_tests/test_build_url_properties.py` * `tests/hypothesis_tests/test_clean_list_properties.py` * `tests/hypothesis_tests/test_comma_separated_list_properties.py` * `tests/hypothesis_tests/test_config_instances_properties.py` * `tests/hypothesis_tests/test_datetime_utils_properties.py` * `tests/hypothesis_tests/test_fetch_closest_option_properties.py` * `tests/hypothesis_tests/test_find_key_by_value_properties.py` * `tests/hypothesis_tests/test_generate_random_string_properties.py` * `tests/hypothesis_tests/test_height_to_cm_properties.py` * `tests/hypothesis_tests/test_is_bool_properties.py` * `tests/hypothesis_tests/test_is_email_properties.py` * `tests/hypothesis_tests/test_is_int_enum_properties.py` * `tests/hypothesis_tests/test_is_url_properties.py` * `tests/hypothesis_tests/test_jwt_token_handler_properties.py` * `tests/hypothesis_tests/test_normalize_properties.py` * `tests/hypothesis_tests/test_parse_any_nan_properties.py` * `tests/hypothesis_tests/test_parse_bool_properties.py` * `tests/hypothesis_tests/test_parse_date_properties.py` * `tests/hypothesis_tests/test_parse_int_list_properties.py` * `tests/hypothesis_tests/test_parse_range_properties.py` * `tests/hypothesis_tests/test_result_to_dict_callback_properties.py` * `tests/hypothesis_tests/test_result_to_dict_convert_value_properties.py` * `tests/hypothesis_tests/test_result_to_dict_decorator_properties.py` * `tests/hypothesis_tests/test_result_to_dict_dict_properties.py` * `tests/hypothesis_tests/test_result_to_dict_list_properties.py` * `tests/hypothesis_tests/test_result_to_dict_primitives_properties.py` * `tests/hypothesis_tests/test_result_to_dict_properties_as_dict_flag_properties.py` * `tests/hypothesis_tests/test_result_to_dict_tuple_properties.py` * `tests/hypothesis_tests/test_similarity_properties.py` * `tests/hypothesis_tests/test_string_roundtrip_properties.py` * `tests/hypothesis_tests/test_thread_lockgroup_properties.py` * `tests/hypothesis_tests/test_validation_properties.py` * `tests/test_abstracts_and_simple_classes.py` * `tests/test_cache_handler_memcached.py` * `tests/test_cache_handler_no_cache.py` * `tests/test_cache_handler_redis.py` * `tests/test_core_lib_main.py` * `tests/test_datetime_utils.py` * `tests/test_handle_exceptions.py` * `tests/test_helpers_challenge.py` * `tests/test_jobs.py` * `tests/test_parse_utils_extra.py` * `tests/test_point.py` * `tests/test_rule_validator_helpers.py` * `tests/test_search_connections.py` * `tests/test_user_security.py` * `tests/test_utility_challenge.py` These file types are not supported: * `.gitignore` * `.vscode/settings.json` * `sonar-project.properties`
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
tests/hypothesis_tests/test_observer_properties.py-25-26 (1)
25-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename ambiguous loop variable names to satisfy lint and improve clarity.
Lines 25/28/36 use
l, which triggers Ruff E741 and can fail lint-gated CI.Proposed fix
- for l in listeners: - obs.attach(l) + for listener in listeners: + obs.attach(listener) @@ - for l in listeners: - self.assertEqual(l.events, [('k', 'v')]) + for listener in listeners: + self.assertEqual(listener.events, [('k', 'v')]) @@ - for l in listeners: - obs.attach(l) + for listener in listeners: + obs.attach(listener)Also applies to: 28-29, 36-37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hypothesis_tests/test_observer_properties.py` around lines 25 - 26, Replace the ambiguous single-character loop variable `l` with a more descriptive name like `listener` in all instances where it is used to iterate over listeners (in the loops at lines 25, 28, and 36). This will satisfy the Ruff E741 lint rule and improve code clarity by avoiding confusion with the number 1 or capital letter I. Update both the loop variable declaration and any references to that variable within the loop body.Source: Linters/SAST tools
tests/test_regressions.py-1506-1507 (1)
1506-1507: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit one-line
forloops to satisfy Ruff E701.Lines 1506-1507 use multi-statements on a line; this can fail lint in CI.
Proposed fix
- for t in threads: t.start() - for t in threads: t.join() + for t in threads: + t.start() + for t in threads: + t.join()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_regressions.py` around lines 1506 - 1507, The one-line for loops in the test_regressions.py file (the two for loops iterating over threads) violate the Ruff E701 linting rule. Refactor the for loops that start threads and join threads by placing each statement on its own line: move the statement following the colon to a new line with proper indentation, converting the compact single-line for loop syntax into a standard multi-line for loop format.Source: Linters/SAST tools
tests/test_regressions.py-1315-1317 (1)
1315-1317: 🎯 Functional Correctness | 🟡 MinorUse
assertInto validate both int and enum values match the contract.The docstring explicitly states the status code should be "int-compatible" and work with either a plain
intor anIntEnum. However, lines 1317 and 1332 useassertIs()which enforces identity to the specific enum object, violating this contract. If the implementation returned a plainintinstead ofHTTPStatus, the test would incorrectly fail despite satisfying the documented requirement.Proposed fix
- self.assertIs(cm.exception.status_code, HTTPStatus.CONFLICT) + self.assertIn(cm.exception.status_code, (409, HTTPStatus.CONFLICT)) @@ - self.assertIs(cm.exception.status_code, HTTPStatus.NOT_FOUND) + self.assertIn(cm.exception.status_code, (404, HTTPStatus.NOT_FOUND))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_regressions.py` around lines 1315 - 1317, The test uses assertIs() on line 1317 to check identity with HTTPStatus.CONFLICT, which enforces that the status_code must be the exact enum object. This violates the documented requirement that the value should be "int-compatible" and work with either a plain int or IntEnum. Replace the assertIs() call with assertIn() to validate that cm.exception.status_code matches one of the acceptable values (both the plain int 409 and HTTPStatus.CONFLICT enum), ensuring the test passes regardless of whether the implementation returns a plain int or the enum value. Apply the same fix to line 1332 if it has the same issue.tests/hypothesis_tests/test_jwt_token_handler_properties.py-55-56 (1)
55-56: 🎯 Functional Correctness | 🟡 MinorCatch
Exceptioninstead ofBaseExceptionin the invalid-token assertion.Line 55 catches
BaseException, which can unintentionally catchKeyboardInterruptandSystemExit. Sincehandler.decode()only raises exceptions derived fromException(PyJWT exceptions inherit fromException), useExceptionto ensure the test only catches intended decoding failures.Proposed fix
- with self.assertRaises(BaseException): + with self.assertRaises(Exception): handler.decode(garbage)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hypothesis_tests/test_jwt_token_handler_properties.py` around lines 55 - 56, In the test method at line 55, the assertRaises context manager is catching BaseException when it should catch Exception instead. This is too broad and can unintentionally catch KeyboardInterrupt and SystemExit. Since handler.decode() only raises exceptions derived from Exception (PyJWT exceptions), change the BaseException parameter in the assertRaises call to Exception to ensure the test only catches intended decoding failures and not system-level interrupts.Source: Linters/SAST tools
tests/test_regressions.py-481-482 (1)
481-482: 🎯 Functional Correctness | 🟡 MinorUse
jwt.exceptions.InvalidSignatureErrorfor the bad-signature test.Line 481 currently catches
Exception, which is too broad and can mask unrelated decode failures. Whenverify=Trueand the token signature doesn't match,jwt.decode()raisesjwt.exceptions.InvalidSignatureError(a subclass ofInvalidTokenError). Use the specific exception:Proposed fix
+ import jwt from datetime import timedelta from core_lib.session.jwt_token_handler import JWTTokenHandler handler_a = JWTTokenHandler('secret-a', timedelta(seconds=60)) token = handler_a.encode({'sub': 'user'}) handler_b = JWTTokenHandler('different-secret', timedelta(seconds=60), verify=True) - with self.assertRaises(Exception): + with self.assertRaises(jwt.exceptions.InvalidSignatureError): handler_b.decode(token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_regressions.py` around lines 481 - 482, The assertRaises context manager in the handler_b.decode(token) test is catching the generic Exception class, which is too broad and could mask unrelated errors. Replace Exception with the specific jwt.exceptions.InvalidSignatureError exception that jwt.decode() raises when verify=True and the token signature is invalid. This makes the test more precise and ensures you are only catching the expected exception.Source: Linters/SAST tools
tests/test_misc_coverage.py-149-155 (1)
149-155: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename ambiguous variable
lto avoid lint failure.Line 149 uses
l, which triggers Ruff E741 and can fail lint checks.Proposed fix
- l = _Listener() - obs = Observer(listener=l, listener_type=_Listener) + listener = _Listener() + obs = Observer(listener=listener, listener_type=_Listener) obs.notify('k', 'v') - self.assertEqual(l.last, ('k', 'v')) - obs.detach(l) + self.assertEqual(listener.last, ('k', 'v')) + obs.detach(listener)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_misc_coverage.py` around lines 149 - 155, The variable `l` on line 149 is ambiguous and triggers the Ruff E741 lint error. Rename the variable `l` to a more descriptive name such as `listener` or `listener1` throughout the test method where it appears. Ensure consistency by updating all references to this variable, including the call to `obs.detach(l)` and any other usages in the test.Source: Linters/SAST tools
tests/test_cache_decorator_stress.py-251-251 (1)
251-251: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit semicolon-chained statements to clear Ruff E702 errors.
These lines currently violate Ruff E702 and may fail lint-gated pipelines.
Proposed fix pattern
- fa(); fa(); fb(); fb() + fa() + fa() + fb() + fb()Apply the same split pattern at Lines 322, 324, 386, 401, and 494.
Also applies to: 322-322, 324-324, 386-386, 401-401, 494-494
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_decorator_stress.py` at line 251, The semicolon-chained statements in the test file violate Ruff E702 linting rules. Split each semicolon-separated statement onto its own line, so that each function call (such as fa() and fb()) appears on a separate line. Apply this splitting pattern consistently across all the flagged locations mentioned in the comment to ensure no multiple statements remain on a single line separated by semicolons.Source: Linters/SAST tools
tests/test_cache_decorator_stress.py-295-300 (1)
295-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert unicode presence in the stored cache key, not just count.
The comment says the key should contain unicode, but the assertion only checks key count. Add a direct content assertion so the test verifies its stated behavior.
Proposed fix
stored = list( CoreLib.cache_registry.get(_HANDLER).cached_function_responses.keys() ) self.assertEqual(len(stored), 1) + self.assertIn('éñ', stored[0])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_decorator_stress.py` around lines 295 - 300, The test currently only verifies the count of stored cache keys with self.assertEqual(len(stored), 1), but the comment indicates it should verify that the key contains unicode. After confirming the length equals 1, add a second assertion to check the actual content of the first stored key to verify it contains the expected unicode characters. Access the key from the stored list and assert that unicode content is present in it, ensuring the test validates both the key existence and its unicode content as the comment describes.tests/hypothesis_tests/test_thread_lockgroup_properties.py-32-32 (1)
32-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename ambiguous comprehension variable to satisfy lint checks.
Line 32 uses
l, which triggers Ruff E741 and can fail lint-gated CI.Proposed fix
- self.assertEqual(len({id(l) for l in locks}), len(keys)) + self.assertEqual(len({id(lock_obj) for lock_obj in locks}), len(keys))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hypothesis_tests/test_thread_lockgroup_properties.py` at line 32, In the test_thread_lockgroup_properties.py file, the set comprehension on the assertEqual line uses the variable name l (lowercase L), which is ambiguous and triggers the Ruff E741 linting error. Rename this comprehension variable l to a more descriptive name like lock to satisfy linting requirements. This change should be made in the set comprehension where you iterate through the locks collection with {id(l) for l in locks}.Source: Linters/SAST tools
tests/hypothesis_tests/test_datetime_utils_properties.py-248-251 (1)
248-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the birthday test against leap-day failures.
Line 250 can raise
ValueErroron February 29 when the target year is non-leap, causing date-dependent CI failures.Proposed fix
def test_birthday_today_age_is_today_minus_birth_year(self): today_d = datetime.date.today() - birth = today_d.replace(year=today_d.year - 30) + target_year = today_d.year - 30 + if today_d.month == 2 and today_d.day == 29: + birth = datetime.date(target_year, 2, 28) + else: + birth = today_d.replace(year=target_year) self.assertEqual(age(birth), 30)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hypothesis_tests/test_datetime_utils_properties.py` around lines 248 - 251, The test_birthday_today_age_is_today_minus_birth_year method can fail when the current date is February 29 (a leap day) because replacing the year to a non-leap year will raise a ValueError. Guard the birth date assignment using a try-except block around the date.replace(year=today_d.year - 30) call, and if a ValueError is caught, adjust the birth date to February 28 of that year instead. This ensures the test runs reliably regardless of whether today is a leap day.tests/hypothesis_tests/test_func_utils_properties.py-43-60 (1)
43-60: 🔒 Security & Privacy | 🟡 MinorRemove
exec-based function generation in tests.Lines 47 and 59 use
execto dynamically create test functions. This can be replaced with a fixed static helper function, since both tests rely oninspect.signature()to introspect parameter names and positions rather than requiring dynamic function creation.Replace the exec calls with a static helper function:
Suggested refactor
class TestGetFuncParamsProperties(unittest.TestCase): + `@staticmethod` + def _f( + p0, p1=None, p2=None, p3=None, p4=None, + p5=None, p6=None, p7=None, p8=None, p9=None + ): + pass + `@given`(n=st.integers(min_value=1, max_value=10)) `@SETTINGS` def test_param_index_matches_position(self, n): - # Build a function with n params named p0..p(n-1) param_names = [f'p{i}' for i in range(n)] - source = f"def f({', '.join(param_names)}): pass" - namespace = {} - exec(source, namespace) - f = namespace['f'] + f = self._f for i, name in enumerate(param_names): self.assertEqual(get_func_parameter_index_by_name(f, name), i) `@given`(values=st.lists(st.integers(), min_size=1, max_size=5)) `@SETTINGS` def test_params_as_dict_captures_positional(self, values): - n = len(values) - param_names = [f'p{i}' for i in range(n)] - source = f"def f({', '.join(param_names)}): pass" - namespace = {} - exec(source, namespace) - f = namespace['f'] + param_names = [f'p{i}' for i in range(len(values))] + f = self._f d = get_func_parameters_as_dict(f, *values) for name, val in zip(param_names, values): self.assertEqual(d[name], val)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hypothesis_tests/test_func_utils_properties.py` around lines 43 - 60, The test methods test_params_as_dict_captures_positional and the preceding test both use exec to dynamically generate functions with varying parameter names. Since the tests only inspect function signatures using get_func_parameter_index_by_name rather than requiring truly dynamic runtime function creation, replace both exec blocks with calls to a static helper function. Create a single helper function that accepts a list of parameter names and returns a function with those parameters configured, then call this helper from both test methods instead of using exec and the namespace dictionary pattern.Source: Linters/SAST tools
tests/test_remaining_gaps.py-986-987 (1)
986-987: 🎯 Functional Correctness | 🟡 MinorUse
Exceptioninstead ofBaseExceptionfor invalid JWT decode.The current assertion catches
BaseException, which is overly broad and can mask non-application control-flow exceptions likeKeyboardInterruptorSystemExit. TheJWTTokenHandler.decode()method wrapsjwt.decode()which raises subclasses ofException(e.g.,jwt.exceptions.InvalidTokenErrorand its variants likeDecodeError,InvalidSignatureError). AssertExceptioninstead to match the actual exceptions raised.Suggested change
- with self.assertRaises(BaseException): + with self.assertRaises(Exception): handler.decode('not-a-real-jwt')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_remaining_gaps.py` around lines 986 - 987, The assertRaises statement is catching BaseException which is overly broad and can mask system-level exceptions like KeyboardInterrupt or SystemExit. Since the JWTTokenHandler.decode() method raises subclasses of Exception (such as jwt.exceptions.InvalidTokenError and its variants), replace BaseException with Exception in the assertRaises call that tests handler.decode('not-a-real-jwt') to properly assert for the actual exceptions that the method raises.Source: Linters/SAST tools
tests/hypothesis_tests/test_parse_int_list_properties.py-29-34 (1)
29-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake this property always exercise doubled-comma parsing.
Line 29 allows single-element lists, so Line 32 can produce a string without
',,', which skips the behavior this test is named for. Usemin_size=2to guarantee the targeted path is exercised.Proposed fix
- `@given`(st.lists(st.integers(), min_size=1, max_size=10)) + `@given`(st.lists(st.integers(), min_size=2, max_size=10))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hypothesis_tests/test_parse_int_list_properties.py` around lines 29 - 34, The test `test_extra_empty_segments_ignored` with `min_size=1` in the st.lists parameter allows single-element lists, which when joined with `',,'` will not produce any doubled commas, causing the test to skip the behavior it is designed to test. Change `min_size=1` to `min_size=2` in the `@given(st.lists(st.integers(), min_size=1, max_size=10))` decorator to guarantee that the generated list always contains at least 2 elements, ensuring at least one doubled-comma separator in the string and properly exercising the empty segments filtering behavior.tests/test_cache_handler_ram_full.py-161-166 (1)
161-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis test does not currently hit the “exact threshold” boundary.
Line 165 advances by 11 seconds for a 10-second TTL, so it checks a post-expiry case, not the exact boundary described by the test name/comment. Use +10 seconds to validate the strict boundary behavior.
Proposed fix
- with freeze_time(datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) + datetime.timedelta(seconds=11)): + with freeze_time(datetime.datetime.now(datetime.timezone.utc).replace(tzinfo=None) + datetime.timedelta(seconds=10)): self.assertIsNone(h.get('k'))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_handler_ram_full.py` around lines 161 - 166, The test_expiry_at_exact_threshold method is advancing time by 11 seconds for a 10-second TTL, which checks post-expiry behavior rather than testing the exact boundary condition. Change the timedelta value in the freeze_time call from datetime.timedelta(seconds=11) to datetime.timedelta(seconds=10) so that the test actually validates the strict less-than boundary behavior at the exact expiry threshold as intended by the test name and comment.tests/test_cache_decorator_flags.py-172-172 (1)
172-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit the chained calls on Line 172 to satisfy Ruff E702.
This line currently contains multiple statements separated by semicolons.
Proposed fix
- fn(); fn(); fn() + fn() + fn() + fn()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_decorator_flags.py` at line 172, The line containing chained fn() calls separated by semicolons violates Ruff E702 which disallows multiple statements on a single line. Split the three consecutive fn() function calls into separate lines so each statement appears on its own line, removing the semicolons that currently separate them.Source: Linters/SAST tools
core_lib/web_helpers/require_login_helper.py-16-17 (1)
16-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve view kwargs named
log_exception.These direct
handle_exceptioncalls will still pop a user view kwarg namedlog_exception. Sincerequire_loginnow forwards arbitrary route kwargs through this path, add the same sentinel used byHandleExceptionbefore callinghandle_exception.Proposed fix
- if WebHelpersUtils.get_server_type() == WebHelpersUtils.ServerType.DJANGO: - return handle_exception(func, request, *args, **kwargs) - return handle_exception(func, *args, **kwargs) + handle_kwargs = dict(kwargs) + handle_kwargs[_HANDLE_EXCEPTION_LOG_SENTINEL] = True + if WebHelpersUtils.get_server_type() == WebHelpersUtils.ServerType.DJANGO: + return handle_exception(func, request, *args, **handle_kwargs) + return handle_exception(func, *args, **handle_kwargs)Add the import alongside
handle_exception:-from core_lib.web_helpers.decorators import handle_exception +from core_lib.web_helpers.decorators import _HANDLE_EXCEPTION_LOG_SENTINEL, handle_exception🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core_lib/web_helpers/require_login_helper.py` around lines 16 - 17, The two handle_exception calls in the require_login_helper are passing kwargs directly which could contain a view kwarg named log_exception that would be incorrectly popped. Import the same sentinel value used by HandleException class, then wrap the kwargs passed to both handle_exception calls (on lines passing request and on the line without request) with this sentinel to preserve any log_exception kwarg that should be treated as a view parameter rather than being consumed by handle_exception.core_lib/observer/observer_decorator.py-28-29 (1)
28-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast when
value_param_nameis invalid.Using
resolved.get(...)silently returnsNonefor a typo/nonexistent parameter name, which can produce incorrect observer payloads without signaling a misconfiguration.🔧 Proposed fix
resolved = get_func_parameters_as_dict(func, *args, **kwargs) - value = resolved.get(self.value_param_name) + if self.value_param_name not in resolved: + raise ValueError( + f'Observe.value_param_name `{self.value_param_name}` is not a parameter of `{func.__qualname__}`' + ) + value = resolved[self.value_param_name]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core_lib/observer/observer_decorator.py` around lines 28 - 29, The issue is that using resolved.get(self.value_param_name) on line 29 silently returns None when the parameter name doesn't exist, masking configuration errors. Replace the .get() call with direct dictionary subscript access using resolved[self.value_param_name] so that a KeyError is raised immediately if the value_param_name is invalid or doesn't exist in the resolved parameters, allowing the misconfiguration to fail fast rather than producing incorrect observer payloads.
🧹 Nitpick comments (4)
.vscode/settings.json (1)
2-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid committing account-specific SonarLint connected-mode settings.
Line 3 and Line 4 hard-code a specific SonarCloud connection/project mapping; this is usually user-specific and noisy for other contributors. Prefer keeping this in user settings and documenting setup steps instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.vscode/settings.json around lines 2 - 5, Remove the sonarlint.connectedMode.project configuration block from the .vscode/settings.json file, as this contains account-specific connection and project mappings that should not be committed to the repository. These user-specific SonarLint settings should be stored in user settings instead. Document the SonarLint setup steps in a separate file (such as CONTRIBUTING.md or a setup guide) so contributors can configure their own connections without committing repository-wide changes.tests/test_cache_decorator_stress.py (1)
275-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the default-handler test deterministic.
This test currently treats both success and
ValueErroras pass, which can hide regressions. Set up an explicit default handler inside the test and assert one expected outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_decorator_stress.py` around lines 275 - 287, The test_no_handler_name_uses_default test method lacks determinism by accepting both success and ValueError outcomes equally through its try-except block, which can hide regressions. Register an explicit default handler in the test setup (before or at the beginning of the test method) and then remove the try-except block from the test. Instead, add a single assertEq assertion to verify that calling fn() decorated with Cache returns the expected result, ensuring the test has one deterministic expected outcome rather than accepting multiple different behaviors.tests/test_cache_decorator_flags.py (1)
249-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this test deterministic instead of accepting opposite outcomes.
Line 263–270 currently treats both “works” and “raises
ValueError” as success, so the test can pass while behavior regresses. Set a deterministic precondition (explicit default handler setup or explicit skip) and assert one expected outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache_decorator_flags.py` around lines 249 - 270, The test_handler_name_none_uses_default_registry method currently has a try/except block that accepts two opposite outcomes as success (either the function executes and cache works, or ValueError is raised), making the test non-deterministic and masking potential regressions. Replace the try/except logic with a deterministic approach by explicitly setting up a default handler in the cache registry before running the test, then remove the exception handling and assert the single expected outcome that fn() calls should be cached (calls should have length 1 after two invocations).core_lib/rule_validator/rule_validator.py (1)
116-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve custom-validator failure reasons instead of re-wrapping them.
The raised
PermissionErrorfor a failed validator is currently caught by the surroundingexcept Exceptionand replaced with a generic message.Proposed patch
try: if rule.custom_validator: custom_valid = rule.custom_validator(parsed_value) is_allow_null = parsed_value is None and rule.nullable if custom_valid is not True and not is_allow_null: raise PermissionError(f'Update of key:`{key}` failed by custom validation') - # Same — narrow from BaseException so process signals propagate. + except PermissionError: + raise + # Same — narrow from BaseException so process signals propagate. except Exception as ex: raise PermissionError(f'Error running custom validator with `{key}` and value `{parsed_value}`') from ex🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core_lib/rule_validator/rule_validator.py` around lines 116 - 124, The issue is that the intentional PermissionError raised when custom_valid check fails is being caught by the outer except Exception clause and re-wrapped with a generic message, losing the specific validation failure reason. Restructure the try-except to only wrap the rule.custom_validator(parsed_value) call itself, moving the custom_valid check and its PermissionError raise outside this try-except block, so that only actual exceptions from the validator execution get caught and wrapped, while intentional validation failures preserve their original error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core_lib/cache/cache_handler_memcached.py`:
- Around line 17-31: The truthiness check in the set method when handling the
expire parameter is problematic because datetime.timedelta(0) is falsy and gets
treated the same as None, both resulting in time=0 (never expires). Replace the
conditional check `if expire` with an explicit None check using `if expire is
not None` to distinguish between an explicitly provided zero duration and no
expiration. Additionally, add validation to reject non-positive timedelta values
(zero or negative durations) to prevent unintended infinite cache entries.
In `@core_lib/cache/cache_handler_ram.py`:
- Around line 8-32: The CacheHandlerRam class has a thread-safety issue where
the cached_function_responses dictionary is accessed and modified without
synchronization. Add a threading lock to the __init__ method to protect all
dictionary operations. In the get method where cached_function_responses is
deleted (around line 19), wrap the deletion in the lock and use pop with a
default value instead of del to safely handle cases where the key might have
been removed by another thread. Similarly, in the delete method (around line 29)
and flush_all method (around line 32), wrap the dictionary modifications with
the lock using the same safe deletion pattern. This ensures concurrent calls to
get, set, delete, and flush_all cannot cause KeyError or race conditions.
In `@core_lib/cache/cache_handler_redis.py`:
- Around line 18-33: The set method uses a truthiness check on expire which
treats timedelta(0) as no expiry, causing zero-duration TTLs to be stored
indefinitely. Replace the if expire: check with an explicit check for None (if
expire is not None:) and add validation to reject non-positive timedelta values
to prevent ambiguous expiry semantics. Additionally, convert the timedelta to
seconds using .total_seconds() when passing to the redis_client.set ex parameter
to ensure proper Redis compatibility.
In `@core_lib/connection/sql_alchemy_connection.py`:
- Around line 32-38: The close() method unconditionally calls flush() and
commit() even after a rollback in __exit__, which can reopen unintended
transactions and skip cleanup if those operations fail. Refactor the close()
method to only perform flush() and commit() when a transaction is actually
active, and ensure that the session.close() is guaranteed to execute even if
flush or commit raises an exception. Consider using a try/finally block to
structure this safely, and check the session's transaction state before
attempting to flush and commit.
In `@core_lib/data_layers/data/db/sqlalchemy/types/point.py`:
- Around line 35-37: The validation logic at line 35 only checks if there are
fewer than two numbers with len(numbers) < 2, which allows inputs with three or
more numeric components to pass validation and be silently truncated to the
first two values on line 37. Change the condition from len(numbers) < 2 to
len(numbers) != 2 to ensure the POINT parser rejects any input that does not
have exactly two numeric components, preventing mis-parsing of invalid WKT-like
strings.
In `@core_lib/error_handling/duplicate_error_decorator.py`:
- Around line 26-30: The logger.warning call in the DuplicateErrorHandler is
logging raw IntegrityError payloads (e.orig) which can expose PII like emails or
usernames in centralized logs. Refactor the warning-level log message to only
include constraint and class metadata from the IntegrityError instead of the raw
error details. Move the detailed error information (e.orig or full error) to a
separate debug-level log statement that will only be emitted when debug logging
is enabled. This way, production logs remain clean of sensitive data while
debugging information is still available when needed.
In `@core_lib/rule_validator/helpers.py`:
- Around line 9-12: The coordinate extraction using the `or` operator at lines
9-10 treats zero values as falsy and incorrectly falls back to the alternative
key, causing valid coordinates like latitude 0.0 or longitude 0.0 to be dropped.
Replace the truthiness-based fallback logic in both the latitude and longitude
assignments with explicit None checks using `is not None` to properly
distinguish between missing values and valid zero coordinates.
In `@core_lib/session/jwt_token_handler.py`:
- Line 9: Change the default value of the `verify` parameter in the `__init__`
method of the JWT token handler class from `False` to `True`. This ensures that
JWT signature verification is enabled by default, preventing unauthorized tokens
from being accepted. Verify that this parameter change correctly propagates to
the token decoding logic where `verify_signature` is set (around line 38) so
that signatures are validated unless explicitly disabled for special debug or
non-auth scenarios.
In `@core_lib/web_helpers/decorators.py`:
- Around line 89-90: The `_execute_error_middlewares(exc, func)` call is missing
the request object as a parameter, causing Django failures to lose
request-scoped middleware context. Modify this call to also pass the request
obtained from `_get_request()` as an additional parameter to
`_execute_error_middlewares` so that error middlewares can maintain proper
Django request context when processing failures from `require_login()` calls.
In `@tests/test_cache_decorator_flags.py`:
- Around line 112-117: The cache key templates in the `@Cache` decorator for the
falsy value tests use the repr format specifier `{falsy!r}`, which can produce
strings containing braces (such as `{}` for empty containers) that break string
formatting and cause test failures. Replace the `{falsy!r}` usage in the key
parameter at both test locations (the decorator around line 113 and the
decorator around line 145) with an alternative approach that does not use repr
formatting, such as using the string representation directly or a different
unique identifier that avoids brace characters in the output.
In `@tests/test_datetime_utils.py`:
- Around line 79-87: The year_end() assertion has multiple
datetime.now(timezone.utc) calls that can return different values if execution
crosses the year boundary between calls, causing intermittent test failures. Fix
this by capturing datetime.now(timezone.utc).replace(tzinfo=None) once at the
start of the assertion and reusing that single captured value in both the
year_end() comparison and the expected value calculation to ensure consistent
time values throughout the assertion.
---
Minor comments:
In `@core_lib/observer/observer_decorator.py`:
- Around line 28-29: The issue is that using resolved.get(self.value_param_name)
on line 29 silently returns None when the parameter name doesn't exist, masking
configuration errors. Replace the .get() call with direct dictionary subscript
access using resolved[self.value_param_name] so that a KeyError is raised
immediately if the value_param_name is invalid or doesn't exist in the resolved
parameters, allowing the misconfiguration to fail fast rather than producing
incorrect observer payloads.
In `@core_lib/web_helpers/require_login_helper.py`:
- Around line 16-17: The two handle_exception calls in the require_login_helper
are passing kwargs directly which could contain a view kwarg named log_exception
that would be incorrectly popped. Import the same sentinel value used by
HandleException class, then wrap the kwargs passed to both handle_exception
calls (on lines passing request and on the line without request) with this
sentinel to preserve any log_exception kwarg that should be treated as a view
parameter rather than being consumed by handle_exception.
In `@tests/hypothesis_tests/test_datetime_utils_properties.py`:
- Around line 248-251: The test_birthday_today_age_is_today_minus_birth_year
method can fail when the current date is February 29 (a leap day) because
replacing the year to a non-leap year will raise a ValueError. Guard the birth
date assignment using a try-except block around the
date.replace(year=today_d.year - 30) call, and if a ValueError is caught, adjust
the birth date to February 28 of that year instead. This ensures the test runs
reliably regardless of whether today is a leap day.
In `@tests/hypothesis_tests/test_func_utils_properties.py`:
- Around line 43-60: The test methods test_params_as_dict_captures_positional
and the preceding test both use exec to dynamically generate functions with
varying parameter names. Since the tests only inspect function signatures using
get_func_parameter_index_by_name rather than requiring truly dynamic runtime
function creation, replace both exec blocks with calls to a static helper
function. Create a single helper function that accepts a list of parameter names
and returns a function with those parameters configured, then call this helper
from both test methods instead of using exec and the namespace dictionary
pattern.
In `@tests/hypothesis_tests/test_jwt_token_handler_properties.py`:
- Around line 55-56: In the test method at line 55, the assertRaises context
manager is catching BaseException when it should catch Exception instead. This
is too broad and can unintentionally catch KeyboardInterrupt and SystemExit.
Since handler.decode() only raises exceptions derived from Exception (PyJWT
exceptions), change the BaseException parameter in the assertRaises call to
Exception to ensure the test only catches intended decoding failures and not
system-level interrupts.
In `@tests/hypothesis_tests/test_observer_properties.py`:
- Around line 25-26: Replace the ambiguous single-character loop variable `l`
with a more descriptive name like `listener` in all instances where it is used
to iterate over listeners (in the loops at lines 25, 28, and 36). This will
satisfy the Ruff E741 lint rule and improve code clarity by avoiding confusion
with the number 1 or capital letter I. Update both the loop variable declaration
and any references to that variable within the loop body.
In `@tests/hypothesis_tests/test_parse_int_list_properties.py`:
- Around line 29-34: The test `test_extra_empty_segments_ignored` with
`min_size=1` in the st.lists parameter allows single-element lists, which when
joined with `',,'` will not produce any doubled commas, causing the test to skip
the behavior it is designed to test. Change `min_size=1` to `min_size=2` in the
`@given(st.lists(st.integers(), min_size=1, max_size=10))` decorator to
guarantee that the generated list always contains at least 2 elements, ensuring
at least one doubled-comma separator in the string and properly exercising the
empty segments filtering behavior.
In `@tests/hypothesis_tests/test_thread_lockgroup_properties.py`:
- Line 32: In the test_thread_lockgroup_properties.py file, the set
comprehension on the assertEqual line uses the variable name l (lowercase L),
which is ambiguous and triggers the Ruff E741 linting error. Rename this
comprehension variable l to a more descriptive name like lock to satisfy linting
requirements. This change should be made in the set comprehension where you
iterate through the locks collection with {id(l) for l in locks}.
In `@tests/test_cache_decorator_flags.py`:
- Line 172: The line containing chained fn() calls separated by semicolons
violates Ruff E702 which disallows multiple statements on a single line. Split
the three consecutive fn() function calls into separate lines so each statement
appears on its own line, removing the semicolons that currently separate them.
In `@tests/test_cache_decorator_stress.py`:
- Line 251: The semicolon-chained statements in the test file violate Ruff E702
linting rules. Split each semicolon-separated statement onto its own line, so
that each function call (such as fa() and fb()) appears on a separate line.
Apply this splitting pattern consistently across all the flagged locations
mentioned in the comment to ensure no multiple statements remain on a single
line separated by semicolons.
- Around line 295-300: The test currently only verifies the count of stored
cache keys with self.assertEqual(len(stored), 1), but the comment indicates it
should verify that the key contains unicode. After confirming the length equals
1, add a second assertion to check the actual content of the first stored key to
verify it contains the expected unicode characters. Access the key from the
stored list and assert that unicode content is present in it, ensuring the test
validates both the key existence and its unicode content as the comment
describes.
In `@tests/test_cache_handler_ram_full.py`:
- Around line 161-166: The test_expiry_at_exact_threshold method is advancing
time by 11 seconds for a 10-second TTL, which checks post-expiry behavior rather
than testing the exact boundary condition. Change the timedelta value in the
freeze_time call from datetime.timedelta(seconds=11) to
datetime.timedelta(seconds=10) so that the test actually validates the strict
less-than boundary behavior at the exact expiry threshold as intended by the
test name and comment.
In `@tests/test_misc_coverage.py`:
- Around line 149-155: The variable `l` on line 149 is ambiguous and triggers
the Ruff E741 lint error. Rename the variable `l` to a more descriptive name
such as `listener` or `listener1` throughout the test method where it appears.
Ensure consistency by updating all references to this variable, including the
call to `obs.detach(l)` and any other usages in the test.
In `@tests/test_regressions.py`:
- Around line 1506-1507: The one-line for loops in the test_regressions.py file
(the two for loops iterating over threads) violate the Ruff E701 linting rule.
Refactor the for loops that start threads and join threads by placing each
statement on its own line: move the statement following the colon to a new line
with proper indentation, converting the compact single-line for loop syntax into
a standard multi-line for loop format.
- Around line 1315-1317: The test uses assertIs() on line 1317 to check identity
with HTTPStatus.CONFLICT, which enforces that the status_code must be the exact
enum object. This violates the documented requirement that the value should be
"int-compatible" and work with either a plain int or IntEnum. Replace the
assertIs() call with assertIn() to validate that cm.exception.status_code
matches one of the acceptable values (both the plain int 409 and
HTTPStatus.CONFLICT enum), ensuring the test passes regardless of whether the
implementation returns a plain int or the enum value. Apply the same fix to line
1332 if it has the same issue.
- Around line 481-482: The assertRaises context manager in the
handler_b.decode(token) test is catching the generic Exception class, which is
too broad and could mask unrelated errors. Replace Exception with the specific
jwt.exceptions.InvalidSignatureError exception that jwt.decode() raises when
verify=True and the token signature is invalid. This makes the test more precise
and ensures you are only catching the expected exception.
In `@tests/test_remaining_gaps.py`:
- Around line 986-987: The assertRaises statement is catching BaseException
which is overly broad and can mask system-level exceptions like
KeyboardInterrupt or SystemExit. Since the JWTTokenHandler.decode() method
raises subclasses of Exception (such as jwt.exceptions.InvalidTokenError and its
variants), replace BaseException with Exception in the assertRaises call that
tests handler.decode('not-a-real-jwt') to properly assert for the actual
exceptions that the method raises.
---
Nitpick comments:
In @.vscode/settings.json:
- Around line 2-5: Remove the sonarlint.connectedMode.project configuration
block from the .vscode/settings.json file, as this contains account-specific
connection and project mappings that should not be committed to the repository.
These user-specific SonarLint settings should be stored in user settings
instead. Document the SonarLint setup steps in a separate file (such as
CONTRIBUTING.md or a setup guide) so contributors can configure their own
connections without committing repository-wide changes.
In `@core_lib/rule_validator/rule_validator.py`:
- Around line 116-124: The issue is that the intentional PermissionError raised
when custom_valid check fails is being caught by the outer except Exception
clause and re-wrapped with a generic message, losing the specific validation
failure reason. Restructure the try-except to only wrap the
rule.custom_validator(parsed_value) call itself, moving the custom_valid check
and its PermissionError raise outside this try-except block, so that only actual
exceptions from the validator execution get caught and wrapped, while
intentional validation failures preserve their original error message.
In `@tests/test_cache_decorator_flags.py`:
- Around line 249-270: The test_handler_name_none_uses_default_registry method
currently has a try/except block that accepts two opposite outcomes as success
(either the function executes and cache works, or ValueError is raised), making
the test non-deterministic and masking potential regressions. Replace the
try/except logic with a deterministic approach by explicitly setting up a
default handler in the cache registry before running the test, then remove the
exception handling and assert the single expected outcome that fn() calls should
be cached (calls should have length 1 after two invocations).
In `@tests/test_cache_decorator_stress.py`:
- Around line 275-287: The test_no_handler_name_uses_default test method lacks
determinism by accepting both success and ValueError outcomes equally through
its try-except block, which can hide regressions. Register an explicit default
handler in the test setup (before or at the beginning of the test method) and
then remove the try-except block from the test. Instead, add a single assertEq
assertion to verify that calling fn() decorated with Cache returns the expected
result, ensuring the test has one deterministic expected outcome rather than
accepting multiple different behaviors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9d66389-3c7e-4635-b84c-86bc7b179353
📒 Files selected for processing (119)
.gitignore.vscode/settings.jsoncore_lib/alembic/alembic.pycore_lib/cache/cache_handler.pycore_lib/cache/cache_handler_memcached.pycore_lib/cache/cache_handler_no_cache.pycore_lib/cache/cache_handler_ram.pycore_lib/cache/cache_handler_redis.pycore_lib/connection/connection_factory_registry.pycore_lib/connection/sql_alchemy_connection.pycore_lib/connection/sql_alchemy_connection_factory.pycore_lib/core_lib.pycore_lib/core_lib_main.pycore_lib/data_layers/data/db/join_config/apply_join_configs.pycore_lib/data_layers/data/db/sqlalchemy/types/int_enum.pycore_lib/data_layers/data/db/sqlalchemy/types/point.pycore_lib/data_layers/data_access/db/crud/crud.pycore_lib/data_layers/data_access/db/crud/crud_data_access.pycore_lib/data_layers/data_access/db/crud/crud_soft_data_access.pycore_lib/data_layers/data_access/db/crud/crud_soft_delete_token_data_access.pycore_lib/data_transform/helpers.pycore_lib/error_handling/duplicate_error_decorator.pycore_lib/error_handling/not_found_decorator.pycore_lib/helpers/config_instances.pycore_lib/helpers/files.pycore_lib/helpers/func_utils.pycore_lib/helpers/generate_data.pycore_lib/helpers/parse_utils.pycore_lib/helpers/shell_utils.pycore_lib/helpers/validation.pycore_lib/jobs/job_scheduler.pycore_lib/middleware/middleware.pycore_lib/middleware/middleware_chain.pycore_lib/observer/observer.pycore_lib/observer/observer_decorator.pycore_lib/registry/default_registry.pycore_lib/rule_validator/helpers.pycore_lib/rule_validator/rule_validator.pycore_lib/session/jwt_token_handler.pycore_lib/session/user_security.pycore_lib/web_helpers/decorators.pycore_lib/web_helpers/request_response_helpers.pycore_lib/web_helpers/require_login_helper.pyscripts/run_sonar_local.shsonar-project.propertiestests/hypothesis_tests/__init__.pytests/hypothesis_tests/_settings.pytests/hypothesis_tests/test_any_to_pascal_properties.pytests/hypothesis_tests/test_build_url_properties.pytests/hypothesis_tests/test_cache_decorator_properties.pytests/hypothesis_tests/test_cache_handler_ram_properties.pytests/hypothesis_tests/test_clean_list_properties.pytests/hypothesis_tests/test_comma_separated_list_properties.pytests/hypothesis_tests/test_config_instances_properties.pytests/hypothesis_tests/test_datetime_utils_properties.pytests/hypothesis_tests/test_default_registry_properties.pytests/hypothesis_tests/test_fetch_closest_option_properties.pytests/hypothesis_tests/test_find_key_by_value_properties.pytests/hypothesis_tests/test_float_to_str_properties.pytests/hypothesis_tests/test_func_utils_properties.pytests/hypothesis_tests/test_generate_datetime_properties.pytests/hypothesis_tests/test_generate_email_properties.pytests/hypothesis_tests/test_generate_random_string_properties.pytests/hypothesis_tests/test_height_to_cm_properties.pytests/hypothesis_tests/test_is_bool_properties.pytests/hypothesis_tests/test_is_email_properties.pytests/hypothesis_tests/test_is_int_enum_properties.pytests/hypothesis_tests/test_is_url_properties.pytests/hypothesis_tests/test_jwt_token_handler_properties.pytests/hypothesis_tests/test_middleware_chain_properties.pytests/hypothesis_tests/test_normalize_properties.pytests/hypothesis_tests/test_observer_properties.pytests/hypothesis_tests/test_parse_any_nan_properties.pytests/hypothesis_tests/test_parse_bool_properties.pytests/hypothesis_tests/test_parse_date_properties.pytests/hypothesis_tests/test_parse_int_list_properties.pytests/hypothesis_tests/test_parse_range_properties.pytests/hypothesis_tests/test_result_to_dict_callback_properties.pytests/hypothesis_tests/test_result_to_dict_convert_value_properties.pytests/hypothesis_tests/test_result_to_dict_decorator_properties.pytests/hypothesis_tests/test_result_to_dict_dict_properties.pytests/hypothesis_tests/test_result_to_dict_list_properties.pytests/hypothesis_tests/test_result_to_dict_primitives_properties.pytests/hypothesis_tests/test_result_to_dict_properties_as_dict_flag_properties.pytests/hypothesis_tests/test_result_to_dict_tuple_properties.pytests/hypothesis_tests/test_rule_validator_properties.pytests/hypothesis_tests/test_similarity_properties.pytests/hypothesis_tests/test_string_roundtrip_properties.pytests/hypothesis_tests/test_thread_lockgroup_properties.pytests/hypothesis_tests/test_validation_properties.pytests/test_abstracts_and_simple_classes.pytests/test_alembic.pytests/test_auth_middlewares.pytests/test_cache_decorator_flags.pytests/test_cache_decorator_stress.pytests/test_cache_handler_memcached.pytests/test_cache_handler_no_cache.pytests/test_cache_handler_ram_full.pytests/test_cache_handler_redis.pytests/test_connections_full.pytests/test_core_lib_main.pytests/test_datetime_utils.pytests/test_example_objectscorelib.pytests/test_handle_exceptions.pytests/test_helpers_challenge.pytests/test_jobs.pytests/test_misc_coverage.pytests/test_parse_utils_extra.pytests/test_point.pytests/test_registry_factory_full.pytests/test_regressions.pytests/test_remaining_gaps.pytests/test_result_to_dict_stress.pytests/test_rule_validator_challenge.pytests/test_rule_validator_helpers.pytests/test_search_connections.pytests/test_shell_utils.pytests/test_user_security.pytests/test_utility_challenge.py
Comment on lines
+17
to
+31
| def set(self, key: str, value, expire: Optional[datetime.timedelta]): | ||
| # Accept any JSON-serializable primitive plus dict / list. Float was | ||
| # previously excluded for no clear reason. | ||
| if not isinstance(value, (dict, list, int, float, str)): | ||
| raise ValueError( | ||
| f'result must be a JSON-serializable type ' | ||
| f'(dict, list, int, float, str). got `{type(value)}`' | ||
| ) | ||
| # memcached treats time=0 as "never expires", which is what we want | ||
| # when no explicit expiry was provided. | ||
| self.memcached_client.set( | ||
| key, | ||
| json.dumps(value), | ||
| time=expire.total_seconds() if expire else 0, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
expire truthiness check can silently create non-expiring cache entries.
Line 30 uses if expire, so datetime.timedelta(0) is treated the same as None and mapped to time=0 (forever). Use explicit None handling and reject non-positive durations.
Suggested fix
def set(self, key: str, value, expire: Optional[datetime.timedelta]):
@@
- self.memcached_client.set(
- key,
- json.dumps(value),
- time=expire.total_seconds() if expire else 0,
- )
+ if expire is not None and expire <= datetime.timedelta(0):
+ raise ValueError("expire must be a positive timedelta or None")
+ ttl_seconds = int(expire.total_seconds()) if expire is not None else 0
+ self.memcached_client.set(
+ key,
+ json.dumps(value),
+ time=ttl_seconds,
+ )🧰 Tools
🪛 ast-grep (0.44.0)
[info] 28-28: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/cache/cache_handler_memcached.py` around lines 17 - 31, The
truthiness check in the set method when handling the expire parameter is
problematic because datetime.timedelta(0) is falsy and gets treated the same as
None, both resulting in time=0 (never expires). Replace the conditional check
`if expire` with an explicit None check using `if expire is not None` to
distinguish between an explicitly provided zero duration and no expiration.
Additionally, add validation to reject non-positive timedelta values (zero or
negative durations) to prevent unintended infinite cache entries.
Comment on lines
+8
to
+32
| def __init__(self, *args, **kwargs): | ||
| self.cached_function_responses = {} | ||
|
|
||
| def get(self, key): | ||
| data = self.cached_function_responses.get(key) | ||
| if data: | ||
| if data['expire']: | ||
| set_time_diff = datetime.datetime.utcnow() - data['set_time'] | ||
| if set_time_diff < data['expire']: | ||
| return data['data'] | ||
| else: | ||
| del self.cached_function_responses[key] | ||
| else: | ||
| return data['data'] | ||
| return None | ||
|
|
||
| def set(self, key: str, value, expire: Optional[datetime.timedelta]): | ||
| self.cached_function_responses[key] = {'data': value, 'set_time': datetime.datetime.utcnow(), 'expire': expire} | ||
|
|
||
| def delete(self, key: str): | ||
| if key in self.cached_function_responses: | ||
| del self.cached_function_responses[key] | ||
|
|
||
| def flush_all(self): | ||
| self.cached_function_responses = {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
CacheHandlerRam mutates shared state without synchronization.
Line 19 and Line 29 can race with concurrent delete/flush_all calls, causing KeyError and nondeterministic behavior. Guard map operations with a lock and use pop(..., None) for safe deletion.
Suggested fix
import datetime
+import threading
@@
class CacheHandlerRam(CacheHandler):
def __init__(self, *args, **kwargs):
self.cached_function_responses = {}
+ self._lock = threading.RLock()
@@
def get(self, key):
- data = self.cached_function_responses.get(key)
- if data:
- if data['expire']:
- set_time_diff = datetime.datetime.utcnow() - data['set_time']
- if set_time_diff < data['expire']:
- return data['data']
- else:
- del self.cached_function_responses[key]
- else:
- return data['data']
+ with self._lock:
+ data = self.cached_function_responses.get(key)
+ if data:
+ if data['expire']:
+ set_time_diff = datetime.datetime.utcnow() - data['set_time']
+ if set_time_diff < data['expire']:
+ return data['data']
+ self.cached_function_responses.pop(key, None)
+ else:
+ return data['data']
return None
@@
def set(self, key: str, value, expire: Optional[datetime.timedelta]):
- self.cached_function_responses[key] = {'data': value, 'set_time': datetime.datetime.utcnow(), 'expire': expire}
+ with self._lock:
+ self.cached_function_responses[key] = {'data': value, 'set_time': datetime.datetime.utcnow(), 'expire': expire}
@@
def delete(self, key: str):
- if key in self.cached_function_responses:
- del self.cached_function_responses[key]
+ with self._lock:
+ self.cached_function_responses.pop(key, None)
@@
def flush_all(self):
- self.cached_function_responses = {}
+ with self._lock:
+ self.cached_function_responses = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/cache/cache_handler_ram.py` around lines 8 - 32, The CacheHandlerRam
class has a thread-safety issue where the cached_function_responses dictionary
is accessed and modified without synchronization. Add a threading lock to the
__init__ method to protect all dictionary operations. In the get method where
cached_function_responses is deleted (around line 19), wrap the deletion in the
lock and use pop with a default value instead of del to safely handle cases
where the key might have been removed by another thread. Similarly, in the
delete method (around line 29) and flush_all method (around line 32), wrap the
dictionary modifications with the lock using the same safe deletion pattern.
This ensures concurrent calls to get, set, delete, and flush_all cannot cause
KeyError or race conditions.
Comment on lines
+18
to
+33
| def set(self, key: str, value, expire: Optional[timedelta]): | ||
| # Accept any JSON-serializable primitive plus dict / list. Float was | ||
| # previously excluded for no clear reason — json.dumps handles it. | ||
| if not isinstance(value, (dict, list, int, float, str)): | ||
| raise ValueError( | ||
| f'result must be a JSON-serializable type ' | ||
| f'(dict, list, int, float, str). got `{type(value)}`' | ||
| ) | ||
| serialized = json.dumps(value) | ||
| # Pass `ex=` only when there's a real expiry. The previous default | ||
| # `ex=-1` triggered "ERR invalid expire time" on the Redis server | ||
| # for any cache set without an explicit timedelta. | ||
| if expire: | ||
| self.redis_client.set(key, serialized, ex=expire) | ||
| else: | ||
| self.redis_client.set(key, serialized) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expiry handling conflates None with zero-duration TTL.
Line 30 uses if expire, so timedelta(0) is treated as “no expiry” and stored indefinitely. Handle None explicitly and reject non-positive TTL values to keep expiry semantics predictable.
Suggested fix
def set(self, key: str, value, expire: Optional[timedelta]):
@@
- if expire:
- self.redis_client.set(key, serialized, ex=expire)
+ if expire is not None:
+ if expire <= timedelta(0):
+ raise ValueError("expire must be a positive timedelta or None")
+ self.redis_client.set(key, serialized, ex=expire)
else:
self.redis_client.set(key, serialized)🧰 Tools
🪛 ast-grep (0.44.0)
[info] 25-25: use jsonify instead of json.dumps for JSON output
Context: json.dumps(value)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/cache/cache_handler_redis.py` around lines 18 - 33, The set method
uses a truthiness check on expire which treats timedelta(0) as no expiry,
causing zero-duration TTLs to be stored indefinitely. Replace the if expire:
check with an explicit check for None (if expire is not None:) and add
validation to reject non-positive timedelta values to prevent ambiguous expiry
semantics. Additionally, convert the timedelta to seconds using .total_seconds()
when passing to the redis_client.set ex parameter to ensure proper Redis
compatibility.
Comment on lines
+32
to
38
| # flush BEFORE commit so any pending changes are sent to the DB and | ||
| # included in the transaction. flush() AFTER commit() was a no-op | ||
| # (commit already flushed and closed the transaction). Then close | ||
| # to release the connection. | ||
| self.session.flush() | ||
| self.session.commit() | ||
| self.session.close() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid unconditional flush/commit in close() after exception rollback paths.
Line 26 rolls back in __exit__, but on_exit at Line 29 still calls close(), which always does Line 36-37 (flush() + commit()). That can reopen/commit an unintended transaction and can also skip close() if flush/commit raises.
[suggested fix keeps close guaranteed and only commits when a transaction is active.]
Proposed patch
def close(self):
- # flush BEFORE commit so any pending changes are sent to the DB and
- # included in the transaction. flush() AFTER commit() was a no-op
- # (commit already flushed and closed the transaction). Then close
- # to release the connection.
- self.session.flush()
- self.session.commit()
- self.session.close()
+ try:
+ if self.session.in_transaction():
+ self.session.flush()
+ self.session.commit()
+ finally:
+ self.session.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/connection/sql_alchemy_connection.py` around lines 32 - 38, The
close() method unconditionally calls flush() and commit() even after a rollback
in __exit__, which can reopen unintended transactions and skip cleanup if those
operations fail. Refactor the close() method to only perform flush() and
commit() when a transaction is actually active, and ensure that the
session.close() is guaranteed to execute even if flush or commit raises an
exception. Consider using a try/finally block to structure this safely, and
check the session's transaction state before attempting to flush and commit.
Comment on lines
+35
to
+37
| if len(numbers) < 2: | ||
| raise ValueError(f'Cannot parse POINT string: {point_str!r}') | ||
| first, second = float(numbers[0]), float(numbers[1]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require exactly two numeric components in POINT parsing.
At Line 35, len(numbers) < 2 lets malformed inputs with extra numeric tokens pass (e.g., 3+ numbers), then Line 37 silently truncates to the first two values. That can mis-parse invalid WKT-like strings into valid coordinates.
Suggested fix
- if len(numbers) < 2:
+ if len(numbers) != 2:
raise ValueError(f'Cannot parse POINT string: {point_str!r}')📝 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.
Suggested change
| if len(numbers) < 2: | |
| raise ValueError(f'Cannot parse POINT string: {point_str!r}') | |
| first, second = float(numbers[0]), float(numbers[1]) | |
| if len(numbers) != 2: | |
| raise ValueError(f'Cannot parse POINT string: {point_str!r}') | |
| first, second = float(numbers[0]), float(numbers[1]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/data_layers/data/db/sqlalchemy/types/point.py` around lines 35 - 37,
The validation logic at line 35 only checks if there are fewer than two numbers
with len(numbers) < 2, which allows inputs with three or more numeric components
to pass validation and be silently truncated to the first two values on line 37.
Change the condition from len(numbers) < 2 to len(numbers) != 2 to ensure the
POINT parser rejects any input that does not have exactly two numeric
components, preventing mis-parsing of invalid WKT-like strings.
Comment on lines
9
to
12
| latitude = location.get('lat') or location.get('latitude') | ||
| longitude = location.get('lng') or location.get('longitude') | ||
| return Point.to_point_str(longitude, latitude) | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not use truthiness fallback for coordinates; it drops valid zero values.
At Line 9 and Line 10, location.get('lat') or location.get('latitude') (and lng/longitude) treats 0 as missing. For valid coordinates like { "lat": 0.0, "lng": 0.0 }, this can produce None and generate an invalid POINT string.
Suggested fix
def convert_location(location: Optional[dict]):
if location:
- latitude = location.get('lat') or location.get('latitude')
- longitude = location.get('lng') or location.get('longitude')
+ latitude = location.get('lat')
+ if latitude is None:
+ latitude = location.get('latitude')
+ longitude = location.get('lng')
+ if longitude is None:
+ longitude = location.get('longitude')
+ if latitude is None or longitude is None:
+ return None
return Point.to_point_str(longitude, latitude)
return None📝 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.
Suggested change
| latitude = location.get('lat') or location.get('latitude') | |
| longitude = location.get('lng') or location.get('longitude') | |
| return Point.to_point_str(longitude, latitude) | |
| return None | |
| def convert_location(location: Optional[dict]): | |
| if location: | |
| latitude = location.get('lat') | |
| if latitude is None: | |
| latitude = location.get('latitude') | |
| longitude = location.get('lng') | |
| if longitude is None: | |
| longitude = location.get('longitude') | |
| if latitude is None or longitude is None: | |
| return None | |
| return Point.to_point_str(longitude, latitude) | |
| return None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/rule_validator/helpers.py` around lines 9 - 12, The coordinate
extraction using the `or` operator at lines 9-10 treats zero values as falsy and
incorrectly falls back to the alternative key, causing valid coordinates like
latitude 0.0 or longitude 0.0 to be dropped. Replace the truthiness-based
fallback logic in both the latitude and longitude assignments with explicit None
checks using `is not None` to properly distinguish between missing values and
valid zero coordinates.
| @@ -8,7 +7,10 @@ | |||
|
|
|||
| class JWTTokenHandler(TokenHandler): | |||
| def __init__(self, secret, expiration_time: timedelta, verify: bool = False, algorithm: str = 'HS256'): | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from datetime import datetime, timedelta, timezone
import jwt
token = jwt.encode(
{"sub": "attacker", "role": "admin", "exp": (datetime.now(timezone.utc) + timedelta(minutes=5)).timestamp()},
"secret-a",
algorithm="HS256",
)
print(jwt.decode(
token,
"different-secret",
algorithms=["HS256"],
options={"verify_signature": False, "verify_exp": True},
))
try:
jwt.decode(token, "different-secret", algorithms=["HS256"])
except Exception as exc:
print(type(exc).__name__)
PYRepository: shay-te/core-lib
Length of output: 725
🏁 Script executed:
find . -name "jwt_token_handler.py" -type fRepository: shay-te/core-lib
Length of output: 194
🏁 Script executed:
cat -n core_lib/session/jwt_token_handler.py | head -50Repository: shay-te/core-lib
Length of output: 2546
🏁 Script executed:
cat -n core_lib/session/jwt_token_handler.py | sed -n '50,65p'Repository: shay-te/core-lib
Length of output: 418
Change JWT token verification default from False to True to prevent signature bypass.
The verify parameter defaults to False, which sets verify_signature: False when decoding (line 38). This allows tokens signed with any secret to be accepted, bypassing signature validation. Flip the default to True and require explicit opt-in for non-auth/debug scenarios that need unsigned verification.
Proposed fix
- def __init__(self, secret, expiration_time: timedelta, verify: bool = False, algorithm: str = 'HS256'):
+ def __init__(self, secret, expiration_time: timedelta, verify: bool = True, algorithm: str = 'HS256'):📝 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.
Suggested change
| def __init__(self, secret, expiration_time: timedelta, verify: bool = False, algorithm: str = 'HS256'): | |
| def __init__(self, secret, expiration_time: timedelta, verify: bool = True, algorithm: str = 'HS256'): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/session/jwt_token_handler.py` at line 9, Change the default value of
the `verify` parameter in the `__init__` method of the JWT token handler class
from `False` to `True`. This ensures that JWT signature verification is enabled
by default, preventing unauthorized tokens from being accepted. Verify that this
parameter change correctly propagates to the token decoding logic where
`verify_signature` is set (around line 38) so that signatures are validated
unless explicitly disabled for special debug or non-auth scenarios.
Comment on lines
89
to
90
| # Run middlewares on all failures | ||
| _execute_error_middlewares(exc, func) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward the Django request into error middleware context.
Line 90 always calls _execute_error_middlewares without the request, while _get_request() now returns None for Django. Django failures raised through require_login(..., request, ...) therefore lose request-scoped middleware context.
Proposed direction
-def _execute_error_middlewares(exc, func):
- request = _get_request()
+def _execute_error_middlewares(exc, func, request=None):
+ if request is None:
+ request = _get_request()
@@
- _execute_error_middlewares(exc, func)
+ middleware_request = None
+ if args:
+ try:
+ if WebHelpersUtils.get_server_type() == WebHelpersUtils.ServerType.DJANGO:
+ middleware_request = args[0]
+ except ValueError:
+ middleware_request = None
+ _execute_error_middlewares(exc, func, middleware_request)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/web_helpers/decorators.py` around lines 89 - 90, The
`_execute_error_middlewares(exc, func)` call is missing the request object as a
parameter, causing Django failures to lose request-scoped middleware context.
Modify this call to also pass the request obtained from `_get_request()` as an
additional parameter to `_execute_error_middlewares` so that error middlewares
can maintain proper Django request context when processing failures from
`require_login()` calls.
Comment on lines
+112
to
+117
| @Cache( | ||
| key=f'falsy-{type(falsy).__name__}-{falsy!r}', | ||
| expire=timedelta(seconds=60), | ||
| cache_empty_result=False, | ||
| handler_name=HANDLER, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid repr() in cache key templates here; it can break formatting.
At Line 113 and Line 145, embedding {falsy!r} in the key can produce brace-containing templates (notably for {}), which can raise formatter errors and fail these subtests for the wrong reason.
Proposed fix
- for falsy in (None, '', [], {}, 0, False):
+ for idx, falsy in enumerate((None, '', [], {}, 0, False)):
with self.subTest(falsy=falsy):
@@
`@Cache`(
- key=f'falsy-{type(falsy).__name__}-{falsy!r}',
+ key=f'falsy-{idx}-{type(falsy).__name__}',
expire=timedelta(seconds=60),
cache_empty_result=False,
handler_name=HANDLER,
)
@@
- for falsy, should_cache in [
+ for idx, (falsy, should_cache) in enumerate([
(None, False),
('', True),
([], True),
({}, True),
(0, True),
(False, True),
- ]:
+ ]):
with self.subTest(falsy=falsy):
@@
`@Cache`(
- key=f'fal2-{type(falsy).__name__}-{falsy!r}',
+ key=f'fal2-{idx}-{type(falsy).__name__}',
expire=timedelta(seconds=60),
cache_empty_result=True,
handler_name=HANDLER,
)Also applies to: 144-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_cache_decorator_flags.py` around lines 112 - 117, The cache key
templates in the `@Cache` decorator for the falsy value tests use the repr format
specifier `{falsy!r}`, which can produce strings containing braces (such as `{}`
for empty containers) that break string formatting and cause test failures.
Replace the `{falsy!r}` usage in the key parameter at both test locations (the
decorator around line 113 and the decorator around line 145) with an alternative
approach that does not use repr formatting, such as using the string
representation directly or a different unique identifier that avoids brace
characters in the output.
Comment on lines
+79
to
87
| year_begin(), datetime.now(timezone.utc).replace(tzinfo=None).replace(month=1, day=1, hour=0, minute=0, second=0, microsecond=0) | ||
| ) | ||
| self.assertNotEqual(year_end(), None) | ||
| self.assertEqual( | ||
| year_end(), | ||
| datetime.utcnow().replace( | ||
| year=datetime.utcnow().year + 1, month=1, day=1, hour=0, minute=0, second=0, microsecond=0 | ||
| datetime.now(timezone.utc).replace(tzinfo=None).replace( | ||
| year=datetime.now(timezone.utc).replace(tzinfo=None).year + 1, month=1, day=1, hour=0, minute=0, second=0, microsecond=0 | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify repeated now() sampling inside single assertions in this file
rg -n "datetime\.now\(timezone\.utc\).*datetime\.now\(timezone\.utc\)" tests/test_datetime_utils.py -C2Repository: shay-te/core-lib
Length of output: 1155
🏁 Script executed:
cat -n tests/test_datetime_utils.py | head -200Repository: shay-te/core-lib
Length of output: 8065
Capture now once per assertion to eliminate timing-dependent test failures.
Multiple datetime.now(timezone.utc) calls within single assertions can cause nondeterministic test failures when code execution crosses time boundaries (year boundary at 82-87, hour boundary at 112 and 119, millisecond boundary at 164). Each call may return a different value, causing assertions to fail intermittently.
Problematic patterns:
- Lines 82-87 (year_end): Two separate
datetime.now(timezone.utc)calls — year value differs if execution crosses year boundary - Line 112 (hour_begin): Two separate calls — hour differs if execution crosses hour boundary
- Line 119 (hour_end): Two separate calls — hour differs if execution crosses hour boundary
- Line 164 (timestamp_to_ms): Two separate
datetime.now(timezone.utc).replace(tzinfo=None).timestamp()calls — timestamps differ if execution crosses millisecond boundary
Fix by capturing datetime.now(timezone.utc).replace(tzinfo=None) once and reusing the captured value, as shown in the proposed diffs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_datetime_utils.py` around lines 79 - 87, The year_end() assertion
has multiple datetime.now(timezone.utc) calls that can return different values
if execution crosses the year boundary between calls, causing intermittent test
failures. Fix this by capturing datetime.now(timezone.utc).replace(tzinfo=None)
once at the start of the assertion and reusing that single captured value in
both the year_end() comparison and the expected value calculation to ensure
consistent time values throughout the assertion.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core_lib/data_layers/data_access/db/crud/crud.py (1)
32-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard against empty update payload after removing
id.
data={"id": ...}passes current checks, thenupdated_databecomes empty and is still sent to.update(...). Add a post-filter guard to fail fast with a clear error.Suggested fix
updated_data = {k: v for k, v in updated_data.items() if k != 'id'} + if not updated_data: + raise AssertionError('CRUD.update requires at least one updatable field') with self._db.get() as session: session.query(self._db_entity).filter(self._db_entity.id == id).update(updated_data)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core_lib/data_layers/data_access/db/crud/crud.py` around lines 32 - 38, After filtering out the `id` key from `updated_data` in the dictionary comprehension, add a guard check to ensure `updated_data` is not empty before passing it to the `.update()` method call on the session query. If `updated_data` is empty after removing the `id` key, raise a clear error (such as ValueError) with a descriptive message indicating that the update payload cannot be empty or contain only the `id` field. This prevents the method from silently sending empty update payloads to the database.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core_lib/helpers/files.py`:
- Around line 25-27: The requests.get call in the download function lacks a
timeout parameter, which can cause the request to block indefinitely on network
stalls. Add a timeout parameter to the requests.get(path, stream=True) call to
specify a reasonable timeout duration (in seconds). This ensures the request
fails fast rather than hanging indefinitely and exhausting thread resources.
In `@core_lib/observer/observer_decorator.py`:
- Around line 20-24: The docstring in the `Observe.__call__` method is missing
the opening and closing triple quotes, which causes a syntax error. Wrap the
existing docstring text (the lines describing "Wraps a function with observer
notification hooks" through the Returns section) with triple quotes (""") at the
beginning and end to properly define the docstring before the `@wraps`(func)
decorator.
In `@tests/test_regressions.py`:
- Around line 1065-1078: The test stub methods in the US class have invalid
syntax due to improper indentation. In the methods secure_entry,
from_session_data, and generate_session_data, the docstrings and return
statements are not indented as part of the method body, causing an
IndentationError. Indent the docstring and return statement for each of these
three methods so they are properly nested within the method definition, ensuring
they align with the correct indentation level for method body content.
---
Outside diff comments:
In `@core_lib/data_layers/data_access/db/crud/crud.py`:
- Around line 32-38: After filtering out the `id` key from `updated_data` in the
dictionary comprehension, add a guard check to ensure `updated_data` is not
empty before passing it to the `.update()` method call on the session query. If
`updated_data` is empty after removing the `id` key, raise a clear error (such
as ValueError) with a descriptive message indicating that the update payload
cannot be empty or contain only the `id` field. This prevents the method from
silently sending empty update payloads to the database.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ffc935a-6df5-4b15-a356-492ab7c9179c
📒 Files selected for processing (61)
core_lib/alembic/alembic.pycore_lib/cache/cache_handler.pycore_lib/cache/cache_handler_memcached.pycore_lib/cache/cache_handler_no_cache.pycore_lib/cache/cache_handler_ram.pycore_lib/cache/cache_handler_redis.pycore_lib/connection/connection_factory_registry.pycore_lib/connection/sql_alchemy_connection.pycore_lib/connection/sql_alchemy_connection_factory.pycore_lib/core_lib.pycore_lib/core_lib_main.pycore_lib/data_layers/data/db/sqlalchemy/types/int_enum.pycore_lib/data_layers/data/db/sqlalchemy/types/point.pycore_lib/data_layers/data_access/db/crud/crud.pycore_lib/data_layers/data_access/db/crud/crud_data_access.pycore_lib/data_layers/data_access/db/crud/crud_soft_data_access.pycore_lib/data_layers/data_access/db/crud/crud_soft_delete_token_data_access.pycore_lib/data_transform/helpers.pycore_lib/error_handling/duplicate_error_decorator.pycore_lib/error_handling/not_found_decorator.pycore_lib/helpers/config_instances.pycore_lib/helpers/files.pycore_lib/helpers/func_utils.pycore_lib/helpers/generate_data.pycore_lib/helpers/parse_utils.pycore_lib/helpers/shell_utils.pycore_lib/jobs/job_scheduler.pycore_lib/middleware/middleware_chain.pycore_lib/observer/observer.pycore_lib/observer/observer_decorator.pycore_lib/registry/default_registry.pycore_lib/rule_validator/helpers.pycore_lib/rule_validator/rule_validator.pycore_lib/session/jwt_token_handler.pycore_lib/session/user_security.pycore_lib/web_helpers/decorators.pycore_lib/web_helpers/request_response_helpers.pycore_lib/web_helpers/require_login_helper.pytests/hypothesis_tests/test_cache_decorator_properties.pytests/hypothesis_tests/test_cache_handler_ram_properties.pytests/hypothesis_tests/test_default_registry_properties.pytests/hypothesis_tests/test_float_to_str_properties.pytests/hypothesis_tests/test_func_utils_properties.pytests/hypothesis_tests/test_generate_datetime_properties.pytests/hypothesis_tests/test_generate_email_properties.pytests/hypothesis_tests/test_middleware_chain_properties.pytests/hypothesis_tests/test_observer_properties.pytests/hypothesis_tests/test_rule_validator_properties.pytests/test_alembic.pytests/test_auth_middlewares.pytests/test_cache_decorator_flags.pytests/test_cache_decorator_stress.pytests/test_cache_handler_ram_full.pytests/test_connections_full.pytests/test_misc_coverage.pytests/test_registry_factory_full.pytests/test_regressions.pytests/test_remaining_gaps.pytests/test_result_to_dict_stress.pytests/test_rule_validator_challenge.pytests/test_shell_utils.py
✅ Files skipped from review due to trivial changes (1)
- core_lib/helpers/shell_utils.py
🚧 Files skipped from review as they are similar to previous changes (52)
- core_lib/cache/cache_handler.py
- core_lib/connection/sql_alchemy_connection_factory.py
- core_lib/core_lib_main.py
- tests/hypothesis_tests/test_generate_email_properties.py
- core_lib/data_layers/data/db/sqlalchemy/types/int_enum.py
- core_lib/error_handling/duplicate_error_decorator.py
- core_lib/error_handling/not_found_decorator.py
- core_lib/helpers/generate_data.py
- core_lib/cache/cache_handler_memcached.py
- tests/hypothesis_tests/test_middleware_chain_properties.py
- core_lib/cache/cache_handler_redis.py
- tests/hypothesis_tests/test_float_to_str_properties.py
- core_lib/data_transform/helpers.py
- tests/hypothesis_tests/test_rule_validator_properties.py
- tests/hypothesis_tests/test_cache_handler_ram_properties.py
- core_lib/rule_validator/helpers.py
- tests/test_cache_handler_ram_full.py
- core_lib/middleware/middleware_chain.py
- core_lib/data_layers/data_access/db/crud/crud_soft_data_access.py
- tests/hypothesis_tests/test_generate_datetime_properties.py
- core_lib/registry/default_registry.py
- core_lib/web_helpers/require_login_helper.py
- core_lib/cache/cache_handler_ram.py
- core_lib/cache/cache_handler_no_cache.py
- core_lib/helpers/func_utils.py
- tests/hypothesis_tests/test_func_utils_properties.py
- core_lib/observer/observer.py
- tests/hypothesis_tests/test_observer_properties.py
- tests/test_rule_validator_challenge.py
- tests/test_auth_middlewares.py
- core_lib/data_layers/data_access/db/crud/crud_soft_delete_token_data_access.py
- tests/test_connections_full.py
- core_lib/session/user_security.py
- tests/hypothesis_tests/test_default_registry_properties.py
- core_lib/session/jwt_token_handler.py
- core_lib/connection/connection_factory_registry.py
- tests/hypothesis_tests/test_cache_decorator_properties.py
- tests/test_registry_factory_full.py
- core_lib/helpers/config_instances.py
- core_lib/web_helpers/decorators.py
- core_lib/rule_validator/rule_validator.py
- tests/test_shell_utils.py
- core_lib/alembic/alembic.py
- core_lib/connection/sql_alchemy_connection.py
- tests/test_cache_decorator_flags.py
- tests/test_cache_decorator_stress.py
- core_lib/helpers/parse_utils.py
- tests/test_result_to_dict_stress.py
- tests/test_alembic.py
- core_lib/jobs/job_scheduler.py
- tests/test_misc_coverage.py
- tests/test_remaining_gaps.py
Comment on lines
+25
to
27
| file = requests.get(path, stream=True) | ||
| with open(local_filename, 'wb') as f: | ||
| download_file_handle(file, f) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the file in question
fd -t f "files.py" | grep -E "helpers|core_lib"Repository: shay-te/core-lib
Length of output: 180
🏁 Script executed:
# Check the actual state of lines 25-27 in the files
cat -n core_lib/helpers/files.py | head -40Repository: shay-te/core-lib
Length of output: 1705
🏁 Script executed:
# Search for timeout usage with requests in the entire codebase
rg "requests\.(get|post|put|delete|request)" --no-heading -A 2 -B 2Repository: shay-te/core-lib
Length of output: 154
🏁 Script executed:
# Check if there's a global timeout configuration or session setup
rg "timeout" core_lib/helpers/files.py -B 3 -A 3Repository: shay-te/core-lib
Length of output: 154
🏁 Script executed:
# Search for Session or timeout configuration anywhere in the project
rg "Session\(\)" -A 5 core_lib/Repository: shay-te/core-lib
Length of output: 512
🏁 Script executed:
# Check if there's any exception handling in the download_file function or related code
rg "download_file" --no-heading -A 10 -B 2Repository: shay-te/core-lib
Length of output: 154
🏁 Script executed:
# Check for any requests configuration or adapter setup
rg "requests\." core_lib/ -A 2 | head -50Repository: shay-te/core-lib
Length of output: 555
Add a timeout to outbound download requests.
requests.get(path, stream=True) lacks a timeout parameter and can block indefinitely on network stalls, risking thread exhaustion and service unavailability.
Suggested fix
- file = requests.get(path, stream=True)
+ file = requests.get(path, stream=True, timeout=30)🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 25-25: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(local_filename, 'wb')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.18)
[error] 25-25: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/helpers/files.py` around lines 25 - 27, The requests.get call in the
download function lacks a timeout parameter, which can cause the request to
block indefinitely on network stalls. Add a timeout parameter to the
requests.get(path, stream=True) call to specify a reasonable timeout duration
(in seconds). This ensures the request fails fast rather than hanging
indefinitely and exhausting thread resources.
Source: Linters/SAST tools
Comment on lines
+20
to
24
| Wraps a function with observer notification hooks. | ||
|
|
||
| Returns: | ||
| A wrapped function that notifies an observer before or after execution, based on the configured timing. | ||
| @wraps(func) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check its content
find . -name "observer_decorator.py" -type f | head -5Repository: shay-te/core-lib
Length of output: 196
🏁 Script executed:
# Read the file to see lines around 20-24
cat -n core_lib/observer/observer_decorator.py | sed -n '15,30p'Repository: shay-te/core-lib
Length of output: 1062
🏁 Script executed:
# Check if the file has valid Python syntax
python3 -m py_compile core_lib/observer/observer_decorator.py 2>&1 || echo "Syntax check complete"Repository: shay-te/core-lib
Length of output: 329
Fix syntax error in Observe.__call__ docstring block.
The function body starts with bare text instead of a valid docstring, which causes a SyntaxError. Add triple quotes around the docstring.
Suggested fix
def __call__(self, func, *args, **kwargs):
- Wraps a function with observer notification hooks.
-
- Returns:
- A wrapped function that notifies an observer before or after execution, based on the configured timing.
+ """
+ Wraps a function with observer notification hooks.
+
+ Returns:
+ A wrapped function that notifies an observer before or after execution, based on the configured timing.
+ """
`@wraps`(func)🧰 Tools
🪛 Ruff (0.15.18)
[warning] 20-20: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 20-20: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 20-20: Compound statements are not allowed on the same line as simple statements
(invalid-syntax)
[warning] 20-20: Expected ,, found name
(invalid-syntax)
[warning] 20-20: Expected ,, found name
(invalid-syntax)
[warning] 20-21: Expected an identifier
(invalid-syntax)
[warning] 22-22: Expected an indented block after with statement
(invalid-syntax)
[warning] 22-23: Expected an expression
(invalid-syntax)
[warning] 23-23: Unexpected indentation
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
[warning] 23-23: Expected ,, found name
(invalid-syntax)
[warning] 23-23: Expected ,, found name
(invalid-syntax)
[warning] 23-23: Expected ,, found name
(invalid-syntax)
[warning] 23-23: Expected ,, found name
(invalid-syntax)
[warning] 23-24: Expected an identifier
(invalid-syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core_lib/observer/observer_decorator.py` around lines 20 - 24, The docstring
in the `Observe.__call__` method is missing the opening and closing triple
quotes, which causes a syntax error. Wrap the existing docstring text (the lines
describing "Wraps a function with observer notification hooks" through the
Returns section) with triple quotes (""") at the beginning and end to properly
define the docstring before the `@wraps`(func) decorator.
Source: Linters/SAST tools
Comment on lines
+1065
to
+1078
| def secure_entry(self, request, session_obj, policies): """ | ||
| Always return None, regardless of request, session, or policies. | ||
|
|
||
| This method serves as a stub or mock implementation for security entry validation. | ||
| """ | ||
| return None | ||
| def from_session_data(self, session_data): """ | ||
| Return the provided session data unchanged. | ||
| """ | ||
| return session_data | ||
| def generate_session_data(self, obj): """ | ||
| Return the input object unchanged. | ||
| """ | ||
| return obj |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
# First, check the file size and read the specific lines
wc -l tests/test_regressions.pyRepository: shay-te/core-lib
Length of output: 185
🏁 Script executed:
# Read the code around lines 1065-1078
sed -n '1060,1085p' tests/test_regressions.py | cat -nRepository: shay-te/core-lib
Length of output: 1230
🏁 Script executed:
# Also read lines 1090-1110 to check the second occurrence
sed -n '1090,1110p' tests/test_regressions.py | cat -nRepository: shay-te/core-lib
Length of output: 977
🏁 Script executed:
# Try to parse the Python file to detect syntax errors
python3 -m py_compile tests/test_regressions.py 2>&1 || trueRepository: shay-te/core-lib
Length of output: 229
Repair invalid method syntax in test stubs (US classes).
The test module fails to parse due to IndentationError at line 1071. Method definitions in the US classes have docstrings and return statements that are not indented as part of the method body.
Suggested fix
class US(UserSecurity):
- def secure_entry(self, request, session_obj, policies): """
-Always return None, regardless of request, session, or policies.
-
-This method serves as a stub or mock implementation for security entry validation.
-"""
-return None
- def from_session_data(self, session_data): """
-Return the provided session data unchanged.
-"""
-return session_data
- def generate_session_data(self, obj): """
-Return the input object unchanged.
-"""
-return obj
+ def secure_entry(self, request, session_obj, policies):
+ """Always return None."""
+ return None
+ def from_session_data(self, session_data):
+ """Return the provided session data unchanged."""
+ return session_data
+ def generate_session_data(self, obj):
+ """Return the input object unchanged."""
+ return objAlso applies to: 1094-1107
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 1071-1071: Unexpected indentation
(invalid-syntax)
[warning] 1074-1074: Expected a statement
(invalid-syntax)
[warning] 1075-1075: Unexpected indentation
(invalid-syntax)
[warning] 1078-1078: Expected a statement
(invalid-syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_regressions.py` around lines 1065 - 1078, The test stub methods in
the US class have invalid syntax due to improper indentation. In the methods
secure_entry, from_session_data, and generate_session_data, the docstrings and
return statements are not indented as part of the method body, causing an
IndentationError. Indent the docstring and return statement for each of these
three methods so they are properly nested within the method definition, ensuring
they align with the correct indentation level for method body content.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🐛 Hypothesis found 2 real bugs
Fixed core_lib/session/jwt_token_handler.py:21 to use datetime.now(tz=timezone.utc). Also updated tests/test_user_security.py:119 which relied on the same buggy assumption.
Cache decorator quirk surfaced
If a cache key template contains { from repr()-ed values, UnseenFormatter.format raises ValueError: expected '}' before end of string. Test reworked to use a clean digits-only key template.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements