refactor: dead code removal + AAD fix + test/doc cleanup#83
Merged
Conversation
Five deserialize_data() calls were missing cache_key=cache_key: - L1+L2 sync path (line 678) - L1+L2 async path (line 973) - L2 async get after lock acquire (line 1035) - L2 async get after lock wait, path A (line 1109) - L2 async get after lock wait, path B (line 1132) Without cache_key, encrypted cache entries using AAD v0x03 would fail verification on every hit, silently falling through to cache miss. The L1-only paths (lines 591, 935) already passed cache_key correctly. Found by code-craftsman expert panel review of remediation spec.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Two tests exercising the thundering herd protection paths where deserialize_data(cache_key=cache_key) was added: 1. Lock acquired, double-check finds cache already populated (another request filled it during lock wait) 2. Lock timeout, final cache check finds it populated (another request completed while waiting for lock) Both use FakeLockableBackend with acquire_lock protocol to trigger the lock-then-deserialize code paths.
27Bslash6
force-pushed
the
refactor/dead-code-removal
branch
from
a5cda04 to
6f582eb
Compare
All 6 tests in this file are redundant or flawed: - test_functional_equivalence_basic: compares @cache to @cache (tautology) - test_configuration_overhead_benchmark: microbenchmark, not correctness - test_intent_profiles_functional: covered by test_intent_decorators.py - test_auto_detection_security_profile: basic @cache usage, covered everywhere - test_concurrent_access_regression: tests thread scheduling nondeterminism, not cachekit behavior (flaky: CI got delta=4 on ±2 tolerance) - test_backward_compatibility_alias: compares @cache to @cache again The file dates from the initial release when @cache was being refactored to the intent-based system. The "regression" it validated is now a tautology since both "legacy" and "intelligent" APIs are the same @cache.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemoved several global convenience APIs and compatibility shims; replaced a module-level global async HTTP client with thread-local client retrieval; removed Changes
sequenceDiagram
participant Client as Client
participant Wrapper as Wrapper
participant Backend as Backend
participant Writer as Writer
Client->>Wrapper: call cached async function (miss)
Wrapper->>Backend: get(key) -> None
Wrapper->>Backend: acquire_lock(key) (await)
alt lock acquired
Wrapper->>Wrapper: compute result
Wrapper->>Wrapper: serialize(result, cache_key=key)
Wrapper->>Wrapper: encrypt(bytes, cache_key=key)
Wrapper->>Backend: set(key, bytes)
Wrapper-->>Client: return result
else lock timeout / held by other
Writer->>Backend: set(key, bytes) (concurrent writer)
Wrapper->>Backend: get(key) -> bytes (double-check)
Wrapper->>Wrapper: deserialize(bytes, cache_key=key)
Wrapper-->>Client: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cachekit/decorators/wrapper.py (2)
1234-1236:⚠️ Potential issue | 🟠 MajorMissing
cache_keyinserialize_data— same AAD binding issue as above.This is the fallback path (no lock or lock failed). Same fix needed for consistency with the encryption security model.
Proposed fix
# Serialize and cache the result try: - serialized_data = operation_handler.serialization_handler.serialize_data(result, args, kwargs) + serialized_data = operation_handler.serialization_handler.serialize_data( + result, args, kwargs, cache_key=cache_key + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/decorators/wrapper.py` around lines 1234 - 1236, The fallback branch where the result is serialized and cached is missing the cache_key argument; update the call to operation_handler.serialization_handler.serialize_data(result, args, kwargs) in the no-lock/fallback path to include the same cache_key used elsewhere (pass cache_key as an additional parameter) so the AAD binding/encryption model remains consistent with other paths that call serialize_data; ensure the call signature matches the serialize_data implementation and any subsequent uses of serialized_data remain unchanged.
1157-1159:⚠️ Potential issue | 🟠 MajorMissing
cache_keyinserialize_data— breaks AAD binding for encryption.The
deserialize_datacalls were fixed to passcache_keyfor AAD verification, but the correspondingserialize_datacall here is missing it. Percache_handler.py(lines 605-612):cache_keyis "SECURITY CRITICAL for encryption" to prevent ciphertext substitution.Without this, encrypted data stored via the lock-acquired path won't have proper AAD binding, causing subsequent
deserialize_datacalls to fail verification.Proposed fix
# Serialize and cache the result try: - serialized_data = operation_handler.serialization_handler.serialize_data(result, args, kwargs) + serialized_data = operation_handler.serialization_handler.serialize_data( + result, args, kwargs, cache_key=cache_key + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/decorators/wrapper.py` around lines 1157 - 1159, The serialize_data call inside the lock-acquired cache write path is missing the cache_key AAD argument, which breaks encryption binding; update the call to operation_handler.serialization_handler.serialize_data(result, args, kwargs, cache_key) (using the same cache_key value used for deserialize_data and earlier cache operations) so ciphertexts include the AAD and subsequent operation_handler.serialization_handler.deserialize_data(...) verification succeeds; ensure you reference the existing cache_key variable rather than creating a different key.src/cachekit/backends/cachekitio/client.py (1)
89-113:⚠️ Potential issue | 🟠 Major
reset_global_client()name is misleading—it only resets the current thread's clients.Because clients are stored in
threading.local(), this function can only clear clients on the calling thread. Any client created on a worker thread will remain alive after a main-thread reset. The function name suggests global cleanup, but it cannot reach clients on other threads. Either:
- Rename the function to reflect current-thread-only semantics (e.g.,
reset_thread_local_clients()), or- Implement centralized client tracking and iterate it to clean up all created instances.
The existing tests at
tests/unit/backends/test_cachekitio_client.py:36–40and123–151validate only same-thread behavior and would not catch cross-thread leaks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/backends/cachekitio/client.py` around lines 89 - 113, The function reset_global_client() is misleading because it only clears thread-local clients stored in _thread_local; rename it to reset_thread_local_clients() (and update its docstring and any callers/tests) to accurately reflect current-thread-only behavior, and keep close_async_client() and close_sync_client() unchanged; if you prefer true cross-thread cleanup instead, implement centralized tracking of created client instances and iterate that registry to close/clear all clients rather than renaming.
🧹 Nitpick comments (2)
tests/unit/test_async_lock_deserialize.py (2)
60-63: Empty fixture may not actually override root conftest behavior.This fixture yields without performing any actual override. If the root conftest's Redis isolation fixture has side effects (e.g., setting environment variables, configuring DI), this won't prevent them.
Consider documenting or implementing actual override
If no override is needed because the test uses an explicit
backend=parameter, add a clarifying comment:`@pytest.fixture`(autouse=True) def setup_di_for_redis_isolation(): - """Override root conftest's Redis isolation.""" + """No-op: Test uses explicit backend= parameter, bypassing DI/Redis lookup.""" yieldOr if actual override is needed, implement it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_async_lock_deserialize.py` around lines 60 - 63, The autouse fixture setup_di_for_redis_isolation currently yields without changing anything and therefore may not override the root conftest Redis isolation; either make the intent explicit by adding a clarifying comment in tests/unit/test_async_lock_deserialize.py that the tests pass an explicit backend= and no override is required, or implement a real override inside setup_di_for_redis_isolation (e.g., monkeypatch environment variables, reset/configure the DI container, or call the root fixture's teardown/register a stub) so it neutralizes the root Redis isolation behavior; reference the fixture name setup_di_for_redis_isolation and the root conftest's Redis isolation when making the change.
75-108: Good coverage for the lock-acquired double-check path.The patching strategy correctly simulates the "cache miss → lock → cache populated" flow. The test validates that
deserialize_data(cache_key=cache_key)at line ~1106 is exercised.Consider tightening the assertion for more precise verification:
Tighten call count assertion
- assert call_count >= 2 + assert call_count == 2, f"Expected exactly 2 get() calls (miss + double-check), got {call_count}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_async_lock_deserialize.py` around lines 75 - 108, Tighten the final assertion in test_lock_acquired_cache_populated_during_wait to precisely verify the simulated flow by asserting call_count == 2 instead of call_count >= 2; update the assertion referencing the patched_get/call_count variables so the test ensures exactly one initial L2 miss and one subsequent L2 hit during the lock-acquired double-check path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/cachekit/backends/cachekitio/client.py`:
- Around line 89-113: The function reset_global_client() is misleading because
it only clears thread-local clients stored in _thread_local; rename it to
reset_thread_local_clients() (and update its docstring and any callers/tests) to
accurately reflect current-thread-only behavior, and keep close_async_client()
and close_sync_client() unchanged; if you prefer true cross-thread cleanup
instead, implement centralized tracking of created client instances and iterate
that registry to close/clear all clients rather than renaming.
In `@src/cachekit/decorators/wrapper.py`:
- Around line 1234-1236: The fallback branch where the result is serialized and
cached is missing the cache_key argument; update the call to
operation_handler.serialization_handler.serialize_data(result, args, kwargs) in
the no-lock/fallback path to include the same cache_key used elsewhere (pass
cache_key as an additional parameter) so the AAD binding/encryption model
remains consistent with other paths that call serialize_data; ensure the call
signature matches the serialize_data implementation and any subsequent uses of
serialized_data remain unchanged.
- Around line 1157-1159: The serialize_data call inside the lock-acquired cache
write path is missing the cache_key AAD argument, which breaks encryption
binding; update the call to
operation_handler.serialization_handler.serialize_data(result, args, kwargs,
cache_key) (using the same cache_key value used for deserialize_data and earlier
cache operations) so ciphertexts include the AAD and subsequent
operation_handler.serialization_handler.deserialize_data(...) verification
succeeds; ensure you reference the existing cache_key variable rather than
creating a different key.
---
Nitpick comments:
In `@tests/unit/test_async_lock_deserialize.py`:
- Around line 60-63: The autouse fixture setup_di_for_redis_isolation currently
yields without changing anything and therefore may not override the root
conftest Redis isolation; either make the intent explicit by adding a clarifying
comment in tests/unit/test_async_lock_deserialize.py that the tests pass an
explicit backend= and no override is required, or implement a real override
inside setup_di_for_redis_isolation (e.g., monkeypatch environment variables,
reset/configure the DI container, or call the root fixture's teardown/register a
stub) so it neutralizes the root Redis isolation behavior; reference the fixture
name setup_di_for_redis_isolation and the root conftest's Redis isolation when
making the change.
- Around line 75-108: Tighten the final assertion in
test_lock_acquired_cache_populated_during_wait to precisely verify the simulated
flow by asserting call_count == 2 instead of call_count >= 2; update the
assertion referencing the patched_get/call_count variables so the test ensures
exactly one initial L2 miss and one subsequent L2 hit during the lock-acquired
double-check path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f73de839-b031-4c38-ace9-b32b2404f51d
📒 Files selected for processing (8)
src/cachekit/backends/cachekitio/client.pysrc/cachekit/decorators/session.pysrc/cachekit/decorators/wrapper.pysrc/cachekit/logging.pysrc/cachekit/reliability/metrics_collection.pytests/critical/test_intelligent_cache_regression.pytests/unit/backends/test_cachekitio_client.pytests/unit/test_async_lock_deserialize.py
💤 Files with no reviewable changes (4)
- src/cachekit/logging.py
- src/cachekit/reliability/metrics_collection.py
- tests/critical/test_intelligent_cache_regression.py
- src/cachekit/decorators/session.py
All date to the v0.1.0-alpha initial commit (a0800c3, 2025-12-11). None were ever called from production code. Removed: - cache_handler.py: get_client_provider(), _get_cached_utility_function(), SerializerFactory class (superseded by get_serializer()) - hash_utils.py: content_checksum(), short_fingerprint() (one-liner wrappers around secure_hash/fast_hash) - metrics_collection.py: record_async_metric(), clear_metrics() - serializers/__init__.py: module-level serialize(), deserialize() - auto_serializer.py: module-level serialize(), deserialize()
…torFeatures alias - safe_mode: accepted on create_cache_wrapper() and DecoratorConfig but silently ignored since initial commit. No deprecation warning, just dead. - record_hit(): deprecated in favor of record_l1_hit/record_l2_hit, zero callers in src/. - DecoratorFeatures = FeatureOrchestrator: backward-compat alias in a greenfield project with no backwards compatibility requirement.
- test_config_advanced.py.obsolete: 432 lines importing nonexistent APIs (load_config_from_file, load_config_from_dict, etc.) - test_reliability_smoke.py: 294-line duplicate of test_reliability_infrastructure_smoke.py in tests/critical/ - test_serializer_validation_integration.py: 169 lines testing the removed SerializerFactory class
Empty tests deleted (inflated test count with zero signal): - 19 docstring-only competitive comparison tests (aiocache, cachetools, lru_cache) - 2 print-only performance summary tests (zero assertions) - 1 assert-True test (test_redis_version_requirement) Fixed: - test_aad_v03_security: added missing assertion for different-key ciphertext - DecoratorFeatures → FeatureOrchestrator in 2 integration tests - Removed safe_mode references from 8 test files (param no longer exists) - Removed clear_metrics/record_async_metric from 2 test files
- DefaultSerializer → StandardSerializer (40+ occurrences across 12 files) - default_serializer.py → standard_serializer.py (6 file path refs) - CachedRedisClientProvider → get_cached_redis_client() (fictional class) - CacheHandler → StandardCacheHandler (wrong import in performance.md) - key_rotation_enabled parameter removed (fictional, never existed)
27Bslash6
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/competitive/test_aiocache_comparison.py (1)
119-131:⚠️ Potential issue | 🟡 MinorAvoid no-op claim tests that always pass.
Line 130 redirects coverage, but this test has no assertions/execution and passes unconditionally. Make delegation explicit (skip) or add a real assertion here.
Suggested minimal fix (explicit delegation)
def test_cachekit_has_zero_knowledge_encryption(self): @@ - # `@cache.secure` enables encryption - # Requires CACHEKIT_MASTER_KEY environment variable - # This is validated in tests/critical/test_aad_v03_security.py + pytest.skip( + "Encryption claim is validated in tests/critical/test_aad_v03_security.py; " + "keep this suite focused on competitive behavior." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/competitive/test_aiocache_comparison.py` around lines 119 - 131, The test function test_cachekit_has_zero_knowledge_encryption is a no-op and always passes; make the intent explicit by either skipping it or adding a real assertion: modify test_cachekit_has_zero_knowledge_encryption to call pytest.skip with a short reason delegating coverage to tests/critical/test_aad_v03_security.py, or replace the no-op with an actual assertion that verifies the encryption feature (e.g., check that CACHEKIT_MASTER_KEY is present or that cache.secure() returns an encrypted wrapper) so the test no longer silently passes.src/cachekit/config/decorator.py (1)
139-188:⚠️ Potential issue | 🟡 MinorSync API docs with
safe_moderemoval
DecoratorConfigno longer exposessafe_mode, butdocs/api-reference.mdstill documents and examples it (docs/api-reference.md, Lines 73-93). This will direct users to pass an unsupported parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/config/decorator.py` around lines 139 - 188, The docs still reference the removed safe_mode parameter for DecoratorConfig; update the API docs and examples to remove all mentions/usages of safe_mode and, where examples or parameter lists relied on it, replace with the current supported configuration options (e.g., show ttl, namespace, serializer, integrity_checking, backend, l1, encryption, etc.) and ensure the DecoratorConfig signature and examples match the code path that defines DecoratorConfig so users aren't instructed to pass an unsupported safe_mode argument.
🧹 Nitpick comments (4)
tests/critical/test_aad_v03_security.py (1)
195-201: Strengthen what this test proves about cache-key binding.Line 201 checks ciphertext inequality, but that can pass from random nonce variation alone. This test currently doesn’t prove
cache_keyis part of authentication.Suggested update
- ciphertext_1, _ = wrapper.serialize(test_data, cache_key="cache:key:1") - ciphertext_2, _ = wrapper.serialize(test_data, cache_key="cache:key:2") + ciphertext_1, metadata_1 = wrapper.serialize(test_data, cache_key="cache:key:1") + ciphertext_2, metadata_2 = wrapper.serialize(test_data, cache_key="cache:key:2") @@ assert ciphertext_1 != ciphertext_2, "Same data encrypted with different keys must produce different ciphertext" + # Ensure ciphertext is bound to its original cache_key + assert wrapper.deserialize(ciphertext_1, metadata_1, cache_key="cache:key:1") == test_data + assert wrapper.deserialize(ciphertext_2, metadata_2, cache_key="cache:key:2") == test_data + with pytest.raises(EncryptionError): + wrapper.deserialize(ciphertext_1, metadata_1, cache_key="cache:key:2") + with pytest.raises(EncryptionError): + wrapper.deserialize(ciphertext_2, metadata_2, cache_key="cache:key:1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/critical/test_aad_v03_security.py` around lines 195 - 201, The test currently only asserts ciphertext inequality which can be due to nonce randomness; instead verify cache-key binding by asserting successful decryption when using the original cache_key and a decryption/auth-failure when using a different cache_key: after calling wrapper.serialize(test_data, cache_key="cache:key:1") and wrapper.serialize(... "cache:key:2"), call wrapper.deserialize on ciphertext_1 with cache_key="cache:key:1" and assert it returns the original test_data, then call wrapper.deserialize on ciphertext_1 with cache_key="cache:key:2" (or an empty/wrong cache_key) and assert it raises an authentication/decryption error (or returns a failure indicator). This uses wrapper.serialize and wrapper.deserialize and the cache_key parameters to prove the AAD is enforced.src/cachekit/decorators/main.py (1)
7-7: Consider a deprecation bridge for the removed public aliasLine 7 finalizes removal of
DecoratorFeaturesfrom the public surface. If this ships outside a major-version break, downstream imports can fail immediately. Consider keeping a temporary alias export for one release with a deprecation warning.Suggested compatibility bridge
from .intent import cache from .orchestrator import FeatureOrchestrator +DecoratorFeatures = FeatureOrchestrator # temporary compatibility alias # Export the intelligent cache interface -__all__ = ["FeatureOrchestrator", "cache"] +__all__ = ["FeatureOrchestrator", "DecoratorFeatures", "cache"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/decorators/main.py` at line 7, Add a temporary compatibility bridge by reintroducing the removed public alias DecoratorFeatures alongside FeatureOrchestrator and cache: export DecoratorFeatures as an alias of FeatureOrchestrator and include a runtime deprecation warning when DecoratorFeatures is imported/used; update __all__ to include "DecoratorFeatures" so downstream imports don't break and ensure the deprecation message clearly instructs users to switch to FeatureOrchestrator and that the alias will be removed in the next major release.tests/unit/test_consolidated_decorator.py (1)
652-656: Test intent is now slightly misnamedAfter Line 655 removed
safe_modepathing, this test mostly validates normal default serialization, not an explicit “fallback” scenario. Consider renaming for clarity (or inject a serializer failure to keep true fallback coverage).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_consolidated_decorator.py` around lines 652 - 656, The test named test_serialization_fallback_behavior no longer exercises a fallback path after safe_mode removal; update it to match intent by either renaming the test (e.g., test_default_serialization_behavior or test_serialization_uses_default_when_available) to reflect it now validates normal default serialization for the cached function_with_serialization (decorated with `@cache`), or change the setup to force a serializer failure (e.g., monkeypatch or register a dummy serializer that raises when selected, then assert that the cache logic falls back to the default serializer) so the test again covers an actual fallback scenario.tests/integration/test_reliability_simple.py (1)
102-123: Consider removing this obsolete skipped test.This test expects
ConnectionErrorto be raised on cache failure, but with the removal ofsafe_mode, the wrapper now unconditionally catches exceptions and falls back to executing the function directly (per wrapper.py lines 764-778). The expected behavior would never occur.Since this test is already skipped and tests behavior that no longer exists in the codebase, consider deleting it entirely rather than keeping dead test code. The skip reason acknowledges the test is obsolete ("fail_closed only applies to circuit breaker states").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_reliability_simple.py` around lines 102 - 123, The skipped test test_fail_closed_strategy in tests/integration/test_reliability_simple.py is obsolete because the wrapper now always catches cache exceptions; delete the entire test function (including the nested fail_closed_func definition and the patch of CacheOperationHandler.get_cached_value) and remove any now-unused imports related only to that test so the test suite no longer contains dead/skipped code referencing fail_closed behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/serializers/encryption.md`:
- Line 60: Update the docs to reflect the actual default serializer used by
EncryptionWrapper: change the table entry that lists StandardSerializer as the
default to AutoSerializer and add a brief note that EncryptionWrapper defaults
to AutoSerializer (which enables Python-specific optimizations like
NumPy/pandas) as implemented in encryption_wrapper.py (see the self.serializer =
serializer if serializer is not None else AutoSerializer() line and the class
docstring). Ensure any surrounding text that describes "default inner
serializer" is consistent with AutoSerializer rather than StandardSerializer.
---
Outside diff comments:
In `@src/cachekit/config/decorator.py`:
- Around line 139-188: The docs still reference the removed safe_mode parameter
for DecoratorConfig; update the API docs and examples to remove all
mentions/usages of safe_mode and, where examples or parameter lists relied on
it, replace with the current supported configuration options (e.g., show ttl,
namespace, serializer, integrity_checking, backend, l1, encryption, etc.) and
ensure the DecoratorConfig signature and examples match the code path that
defines DecoratorConfig so users aren't instructed to pass an unsupported
safe_mode argument.
In `@tests/competitive/test_aiocache_comparison.py`:
- Around line 119-131: The test function
test_cachekit_has_zero_knowledge_encryption is a no-op and always passes; make
the intent explicit by either skipping it or adding a real assertion: modify
test_cachekit_has_zero_knowledge_encryption to call pytest.skip with a short
reason delegating coverage to tests/critical/test_aad_v03_security.py, or
replace the no-op with an actual assertion that verifies the encryption feature
(e.g., check that CACHEKIT_MASTER_KEY is present or that cache.secure() returns
an encrypted wrapper) so the test no longer silently passes.
---
Nitpick comments:
In `@src/cachekit/decorators/main.py`:
- Line 7: Add a temporary compatibility bridge by reintroducing the removed
public alias DecoratorFeatures alongside FeatureOrchestrator and cache: export
DecoratorFeatures as an alias of FeatureOrchestrator and include a runtime
deprecation warning when DecoratorFeatures is imported/used; update __all__ to
include "DecoratorFeatures" so downstream imports don't break and ensure the
deprecation message clearly instructs users to switch to FeatureOrchestrator and
that the alias will be removed in the next major release.
In `@tests/critical/test_aad_v03_security.py`:
- Around line 195-201: The test currently only asserts ciphertext inequality
which can be due to nonce randomness; instead verify cache-key binding by
asserting successful decryption when using the original cache_key and a
decryption/auth-failure when using a different cache_key: after calling
wrapper.serialize(test_data, cache_key="cache:key:1") and wrapper.serialize(...
"cache:key:2"), call wrapper.deserialize on ciphertext_1 with
cache_key="cache:key:1" and assert it returns the original test_data, then call
wrapper.deserialize on ciphertext_1 with cache_key="cache:key:2" (or an
empty/wrong cache_key) and assert it raises an authentication/decryption error
(or returns a failure indicator). This uses wrapper.serialize and
wrapper.deserialize and the cache_key parameters to prove the AAD is enforced.
In `@tests/integration/test_reliability_simple.py`:
- Around line 102-123: The skipped test test_fail_closed_strategy in
tests/integration/test_reliability_simple.py is obsolete because the wrapper now
always catches cache exceptions; delete the entire test function (including the
nested fail_closed_func definition and the patch of
CacheOperationHandler.get_cached_value) and remove any now-unused imports
related only to that test so the test suite no longer contains dead/skipped code
referencing fail_closed behavior.
In `@tests/unit/test_consolidated_decorator.py`:
- Around line 652-656: The test named test_serialization_fallback_behavior no
longer exercises a fallback path after safe_mode removal; update it to match
intent by either renaming the test (e.g., test_default_serialization_behavior or
test_serialization_uses_default_when_available) to reflect it now validates
normal default serialization for the cached function_with_serialization
(decorated with `@cache`), or change the setup to force a serializer failure
(e.g., monkeypatch or register a dummy serializer that raises when selected,
then assert that the cache logic falls back to the default serializer) so the
test again covers an actual fallback scenario.
🪄 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
Run ID: 2db1bea8-099f-4b8e-8a32-299f3a719770
📒 Files selected for processing (43)
.secrets.baselineREADME.mddocs/api-reference.mddocs/data-flow-architecture.mddocs/features/zero-knowledge-encryption.mddocs/performance.mddocs/serializers/README.mddocs/serializers/arrow.mddocs/serializers/custom.mddocs/serializers/encryption.mddocs/serializers/orjson.mddocs/serializers/pydantic.mdsrc/cachekit/cache_handler.pysrc/cachekit/config/decorator.pysrc/cachekit/decorators/__init__.pysrc/cachekit/decorators/main.pysrc/cachekit/decorators/wrapper.pysrc/cachekit/hash_utils.pysrc/cachekit/reliability/metrics_collection.pysrc/cachekit/serializers/__init__.pysrc/cachekit/serializers/auto_serializer.pysrc/cachekit/serializers/base.pysrc/cachekit/serializers/orjson_serializer.pytests/competitive/test_aiocache_comparison.pytests/competitive/test_cachetools_comparison.pytests/competitive/test_lru_cache_comparison.pytests/critical/test_aad_v03_security.pytests/critical/test_reliability_infrastructure_smoke.pytests/critical/test_serializer_validation_integration.pytests/docs/test_ground_truth.pytests/integration/test_reliability_simple.pytests/performance/test_end_to_end_latency.pytests/performance/test_production_realism.pytests/test_decorator_intent_examples.pytests/unit/config/test_decorator_config.pytests/unit/config/test_presets.pytests/unit/test_adaptive_timeout_integration.pytests/unit/test_backpressure_integration.pytests/unit/test_config_advanced.py.obsoletetests/unit/test_consolidated_decorator.pytests/unit/test_metrics_collection.pytests/unit/test_reliability_fallback.pytests/unit/test_reliability_smoke.py
💤 Files with no reviewable changes (16)
- tests/performance/test_end_to_end_latency.py
- tests/test_decorator_intent_examples.py
- tests/critical/test_reliability_infrastructure_smoke.py
- tests/competitive/test_lru_cache_comparison.py
- tests/docs/test_ground_truth.py
- src/cachekit/hash_utils.py
- tests/unit/test_metrics_collection.py
- tests/unit/config/test_presets.py
- src/cachekit/serializers/init.py
- tests/competitive/test_cachetools_comparison.py
- src/cachekit/serializers/auto_serializer.py
- tests/unit/test_config_advanced.py.obsolete
- src/cachekit/reliability/metrics_collection.py
- src/cachekit/cache_handler.py
- tests/unit/test_reliability_smoke.py
- tests/critical/test_serializer_validation_integration.py
✅ Files skipped from review due to trivial changes (13)
- docs/serializers/custom.md
- src/cachekit/serializers/orjson_serializer.py
- docs/serializers/pydantic.md
- docs/performance.md
- README.md
- docs/features/zero-knowledge-encryption.md
- docs/serializers/README.md
- docs/api-reference.md
- src/cachekit/serializers/base.py
- docs/serializers/arrow.md
- .secrets.baseline
- docs/serializers/orjson.md
- docs/data-flow-architecture.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cachekit/decorators/wrapper.py
Previously, EncryptionWrapper(enable_encryption=False) silently downgraded to plaintext with a log warning — a security violation. An encryption wrapper that doesn't encrypt is a misconfiguration, not a valid operating mode. Now raises EncryptionError if enable_encryption=False. Also: - Default inner serializer confirmed as StandardSerializer (cross-language MessagePack) for multi-client key sharing via secrets manager - Removed dead AutoSerializer import - Fixed registry comment: "AutoSerializer + AES-256-GCM" → "StandardSerializer"
Docs: - encryption.md: default inner serializer is AutoSerializer → StandardSerializer - api-reference.md: remove safe_mode from parameter list and example Tests: - AAD test: add cross-key deserialization failure assertion (proves AAD binding) - aiocache comparison: empty encryption test → pytest.skip with cross-ref - encryption composability: rename misnamed auto_serializer → default_serializer - reliability: delete permanently-skipped test_fail_closed_strategy (dead) - consolidated decorator: rename fallback → default_serialization_behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cachekit/serializers/encryption_wrapper.py (1)
127-133: Good security hardening, but creates dead code paths elsewhere.Raising
EncryptionErrorwhenenable_encryption=Falseis a sound security decision. However, this makes several code paths unreachable:
- Line 224-225:
if not self.enable_encryption: return raw_data, raw_metadata- Line 333-334:
if not self.enable_encryption: raise EncryptionError(...)- Line 551-552:
if not self.enable_encryption: return False- Line 578-579:
if not self.enable_encryption: return {"enabled": False, ...}Since
self.enable_encryptionis alwaysTrueafter a successful__init__, these branches are dead code.♻️ Proposed refactor to remove dead code
Remove the dead branches in
serialize()(Lines 224-225):- # If encryption is disabled, return raw data with modified metadata - if not self.enable_encryption: - return raw_data, raw_metadata - # SECURITY: Validate cache_key type and value when encryption is enabledRemove the dead check in
deserialize()(Lines 333-334):- # Verify encryption is available - if not self.enable_encryption: - raise EncryptionError("Received encrypted data but encryption is disabled") - # Encryption is enabled - encryptor and tenant_keys must be initializedRemove the dead branch in
hardware_acceleration_enabled(Lines 551-552):- if not self.enable_encryption: - return False if self.encryptor is None:Remove the dead branch in
get_encryption_info()(Lines 578-579):- if not self.enable_encryption: - return {"enabled": False, "reason": "Encryption disabled or not available"} - if self.encryption_key_fingerprint is None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/serializers/encryption_wrapper.py` around lines 127 - 133, The __init__ force-enables encryption (calling _setup_encryption or raising), so any subsequent conditional branches checking self.enable_encryption are dead; remove those unreachable checks and simplify the logic in the affected methods: delete the early-return in serialize(), delete the redundant raise in deserialize(), remove the unconditional False branch in hardware_acceleration_enabled(), and remove the disabled-flag branch in get_encryption_info(); update those methods to assume encryption is enabled and keep only the real encryption logic paths (refer to methods serialize, deserialize, hardware_acceleration_enabled, get_encryption_info and the __init__/_setup_encryption flow).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 106-108: The docstring for the enable_encryption parameter in the
constructor (around line 107) is now outdated and misleading. It states that
encryption can be disabled for testing purposes, but the constructor now raises
EncryptionError when enable_encryption is False. Update the docstring for the
enable_encryption parameter to accurately reflect that it must be True and
cannot be disabled, removing the reference to testing scenarios where encryption
can be disabled.
---
Nitpick comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 127-133: The __init__ force-enables encryption (calling
_setup_encryption or raising), so any subsequent conditional branches checking
self.enable_encryption are dead; remove those unreachable checks and simplify
the logic in the affected methods: delete the early-return in serialize(),
delete the redundant raise in deserialize(), remove the unconditional False
branch in hardware_acceleration_enabled(), and remove the disabled-flag branch
in get_encryption_info(); update those methods to assume encryption is enabled
and keep only the real encryption logic paths (refer to methods serialize,
deserialize, hardware_acceleration_enabled, get_encryption_info and the
__init__/_setup_encryption flow).
🪄 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
Run ID: 16869a28-6b57-49c3-add3-8c700b95540d
📒 Files selected for processing (9)
docs/api-reference.mddocs/serializers/encryption.mdsrc/cachekit/serializers/__init__.pysrc/cachekit/serializers/encryption_wrapper.pytests/competitive/test_aiocache_comparison.pytests/critical/test_aad_v03_security.pytests/integration/test_encryption_composability.pytests/integration/test_reliability_simple.pytests/unit/test_consolidated_decorator.py
✅ Files skipped from review due to trivial changes (1)
- docs/api-reference.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/serializers/encryption.md
- tests/unit/test_consolidated_decorator.py
- tests/competitive/test_aiocache_comparison.py
- tests/critical/test_aad_v03_security.py
- tests/integration/test_reliability_simple.py
…nWrapper EncryptionWrapper had a mutable `enable_encryption` attribute that could be set to False at runtime, causing serialize() to silently return plaintext through a class named "EncryptionWrapper". Since instances are cached per-tenant in an LRU, one mutation would poison all requests for that tenant. Changes: - Remove `enable_encryption` parameter from __init__ - Remove all `if not self.enable_encryption` bypass branches - Add __slots__ to prevent arbitrary attribute injection - is_encryption_enabled property now returns True unconditionally - Remove enable_encryption=True from 11 test call sites
…UUID fallback F2: When encryption is enabled, reject serializer configurations other than 'default'/'std'/'standard' at config time. Previously, non-default serializers were silently ignored — EncryptionWrapper always uses StandardSerializer internally for cross-SDK interop, but the metadata envelope was incorrectly labeled with the user's requested serializer. F3: Remove unreachable null UUID tenant fallback in serialize_data(). The constructor already validates that either tenant_extractor or single_tenant_mode is set when encryption=True. The else branch with "00000000-..." was dead code. Deserialize path now raises on missing tenant_id instead of falling back to null UUID.
…ined param F5: validate_encryption_config() called reset_settings() on every @cache.secure decoration — a testing convenience leaked into production code. Removed. Tests should manage their own settings resets. F7: create_cache_wrapper() accepted pipelined: bool = True but never referenced it in the function body. 4 comments mentioned it, zero callers passed it, it wasn't on DecoratorConfig. Deleted.
Tests for uncovered patch lines: - EncryptionWrapper with explicit serializer (if serializer is not None) - CacheSerializationHandler rejects non-default serializer with encryption - CacheSerializationHandler accepts default/std/standard aliases - Missing tenant_id in encrypted metadata raises SerializationError
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cachekit/cache_handler.py (1)
36-40:⚠️ Potential issue | 🟠 MajorRestore
CacheClientProviderregistration in the DI container.
src/cachekit/health.py:133-144,325-335, and367-373, plussrc/cachekit/backends/redis/backend.py:71-76, still resolveCacheClientProviderfrom this container. With the registration removed here, those paths now fail with an unregistered-type error.🐛 Proposed fix
from cachekit.backends.provider import ( + CacheClientProvider, BackendProviderInterface, + DefaultCacheClientProvider, DefaultBackendProvider, DefaultLoggerProvider, LoggerProvider, ) @@ container = DIContainer() container.register(LoggerProvider, DefaultLoggerProvider) container.register(BackendProviderInterface, DefaultBackendProvider) +container.register(CacheClientProvider, DefaultCacheClientProvider)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/cache_handler.py` around lines 36 - 40, The DI container currently registers LoggerProvider and BackendProviderInterface but no longer registers CacheClientProvider, causing resolution failures in functions that call container.resolve(CacheClientProvider); restore a registration in the global container (container.register(CacheClientProvider, <appropriate provider class>)) so CacheClientProvider can be resolved; locate the DIContainer instance named container and add a register call for CacheClientProvider mapped to the concrete provider (e.g., DefaultCacheClientProvider or the project's cache provider class) alongside the existing LoggerProvider and BackendProviderInterface registrations.src/cachekit/decorators/wrapper.py (1)
103-192:⚠️ Potential issue | 🟠 Major
record_hit()still has a live caller.
tests/unit/test_cache_info_latency.py:135-150still invokesstats.record_hit("l1")andstats.record_hit("l2"). Removing that shim here turns the suite into anAttributeError. Please either update the remaining call site in this PR or keep a temporary compatibility adapter that delegates torecord_l1_hit()/record_l2_hit().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/decorators/wrapper.py` around lines 103 - 192, The tests still call the removed compatibility method record_hit, so restore a temporary adapter on _FunctionStats named record_hit(self, kind: str, latency_ms: float | None = None) that delegates to record_l1_hit() when kind == "l1" and to record_l2_hit(latency_ms) when kind == "l2" (and raise a clear ValueError for unknown kinds), or alternatively update the remaining test/caller in tests/unit/test_cache_info_latency.py to call record_l1_hit() and record_l2_hit() directly; reference _FunctionStats.record_hit, _FunctionStats.record_l1_hit, and _FunctionStats.record_l2_hit when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cachekit/cache_handler.py`:
- Around line 338-349: The check in EncryptionWrapper (serializer_name) allows
aliases "default", "std", "standard" which later fragments
_serializer_string_name; normalize canonical name before persisting metadata by
mapping "std" and "standard" to "default" (i.e., canonicalize serializer_name to
"default" prior to raising errors or assigning _serializer_string_name/metadata)
so all aliases produce the same envelope identity and cross-SDK readers accept
entries written with any alias.
In `@src/cachekit/decorators/wrapper.py`:
- Line 1015: The async cache-miss/write branches are calling
operation_handler.serialization_handler.serialize_data(result, args, kwargs)
without passing the cache_key, causing EncryptionWrapper.serialize() to reject
empty keys; update every async branch that writes the cache (the serialize_data
calls referenced near the async miss/write logic and the same pattern at the
other occurrences) to include cache_key as a named argument (e.g.,
serialize_data(result, args, kwargs, cache_key=cache_key)), matching the
deserialize call which already passes cache_key, so encrypted caches are
populated correctly.
---
Outside diff comments:
In `@src/cachekit/cache_handler.py`:
- Around line 36-40: The DI container currently registers LoggerProvider and
BackendProviderInterface but no longer registers CacheClientProvider, causing
resolution failures in functions that call
container.resolve(CacheClientProvider); restore a registration in the global
container (container.register(CacheClientProvider, <appropriate provider
class>)) so CacheClientProvider can be resolved; locate the DIContainer instance
named container and add a register call for CacheClientProvider mapped to the
concrete provider (e.g., DefaultCacheClientProvider or the project's cache
provider class) alongside the existing LoggerProvider and
BackendProviderInterface registrations.
In `@src/cachekit/decorators/wrapper.py`:
- Around line 103-192: The tests still call the removed compatibility method
record_hit, so restore a temporary adapter on _FunctionStats named
record_hit(self, kind: str, latency_ms: float | None = None) that delegates to
record_l1_hit() when kind == "l1" and to record_l2_hit(latency_ms) when kind ==
"l2" (and raise a clear ValueError for unknown kinds), or alternatively update
the remaining test/caller in tests/unit/test_cache_info_latency.py to call
record_l1_hit() and record_l2_hit() directly; reference
_FunctionStats.record_hit, _FunctionStats.record_l1_hit, and
_FunctionStats.record_l2_hit when making the change.
🪄 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
Run ID: 7f83e46b-ad09-410d-b381-4f003b93b0c4
📒 Files selected for processing (6)
.secrets.baselinesrc/cachekit/cache_handler.pysrc/cachekit/config/validation.pysrc/cachekit/decorators/wrapper.pysrc/cachekit/serializers/encryption_wrapper.pytests/critical/test_aad_v03_security.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .secrets.baseline
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_encryption_security_invariants.py (1)
76-107: Parametrize duplicated alias-acceptance tests.These two tests are structurally identical except for serializer alias. Parametrizing them reduces duplication and keeps future changes safer.
Refactor sketch
- def test_default_alias_accepted(self, monkeypatch): - """'default' serializer name is accepted with encryption=True.""" - monkeypatch.setenv("CACHEKIT_MASTER_KEY", "a" * 64) - from cachekit.config.singleton import reset_settings - - reset_settings() - try: - handler = CacheSerializationHandler( - serializer_name="default", - encryption=True, - single_tenant_mode=True, - ) - assert handler.encryption is True - finally: - reset_settings() - - def test_std_alias_accepted(self, monkeypatch): - """'std' serializer name is accepted with encryption=True.""" + `@pytest.mark.parametrize`("alias", ["default", "std"]) + def test_default_aliases_accepted(self, monkeypatch, alias): + """Default aliases are accepted with encryption=True.""" monkeypatch.setenv("CACHEKIT_MASTER_KEY", "a" * 64) from cachekit.config.singleton import reset_settings reset_settings() try: handler = CacheSerializationHandler( - serializer_name="std", + serializer_name=alias, encryption=True, single_tenant_mode=True, ) assert handler.encryption is True finally: reset_settings()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_encryption_security_invariants.py` around lines 76 - 107, Parametrize the two identical tests into one parameterized test that iterates over the serializer aliases ("default", "std"); replace test_default_alias_accepted and test_std_alias_accepted with a single test function (e.g., test_alias_accepted_with_encryption) that takes an alias parameter, still sets CACHEKIT_MASTER_KEY via monkeypatch.setenv, calls reset_settings(), constructs CacheSerializationHandler(serializer_name=alias, encryption=True, single_tenant_mode=True), asserts handler.encryption is True, and calls reset_settings() in teardown; keep references to CacheSerializationHandler, serializer_name, encryption, monkeypatch.setenv and reset_settings so the behavior and setup remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_encryption_security_invariants.py`:
- Around line 108-128: The test test_standard_alias_passes_validation_guard
currently swallows all exceptions which can mask unrelated failures; narrow the
broad except Exception in the CacheSerializationHandler(...) block to only the
expected non-ConfigurationError outcomes (e.g., except ValueError:) so the test
only ignores registry/lookup errors and still fails for unexpected exceptions;
update the except clause accordingly near the try/except that constructs
CacheSerializationHandler and keep the ConfigurationError except as-is.
---
Nitpick comments:
In `@tests/unit/test_encryption_security_invariants.py`:
- Around line 76-107: Parametrize the two identical tests into one parameterized
test that iterates over the serializer aliases ("default", "std"); replace
test_default_alias_accepted and test_std_alias_accepted with a single test
function (e.g., test_alias_accepted_with_encryption) that takes an alias
parameter, still sets CACHEKIT_MASTER_KEY via monkeypatch.setenv, calls
reset_settings(), constructs CacheSerializationHandler(serializer_name=alias,
encryption=True, single_tenant_mode=True), asserts handler.encryption is True,
and calls reset_settings() in teardown; keep references to
CacheSerializationHandler, serializer_name, encryption, monkeypatch.setenv and
reset_settings so the behavior and setup remain identical.
🪄 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
Run ID: 6c8ba751-38e3-42b4-a52d-4cd180460d06
📒 Files selected for processing (1)
tests/unit/test_encryption_security_invariants.py
__slots__ prevents attribute deletion. _setup_encryption() sets encryptor, tenant_keys, and encryption_key_fingerprint or raises. The None checks in serialize(), deserialize(), hardware_acceleration, and get_encryption_info were testing states that cannot exist. Also removed redundant None checks inside _setup_encryption (checking encryptor is None 12 lines after setting it, checking .hex() returns None which str.hex() cannot do).
- Merge test_default_alias_accepted + test_std_alias_accepted into single parametrized test_alias_accepted_with_encryption - Narrow except Exception to except ValueError in standard alias test (only expect registry lookup failure, not arbitrary exceptions)
Delete secure_hash() from hash_utils.py — identical to fast_hash() with different default digest_size, zero callers anywhere. Delete test_url_security.py.obsolete — disabled security tests serving no purpose. Fix 3 phantom references missed in the rename sweep: - llms.txt: DefaultSerializer → StandardSerializer - pyproject.toml: default_serializer.py → standard_serializer.py - DEVELOPMENT.md: RawSerializer → StandardSerializer
Add 3 tests for EncryptionWrapper._setup_encryption branches: - Missing master key in settings raises EncryptionError - Malformed hex key in settings raises EncryptionError - Key shorter than 32 bytes raises EncryptionError Covers lines 130, 135-136, 139 in encryption_wrapper.py that codecov flagged as uncovered patch lines.
Full serialize_data/deserialize_data round-trip through CacheSerializationHandler with encryption=True and single_tenant_mode. Covers the serialize encryption branch (lines 559-578) and the deserialize success path with valid tenant_id (lines 667-678) that codecov flagged as uncovered patch lines.
Assert is_encryption_enabled returns True on existing test wrapper instance. Brings patch coverage to ~87% — remaining 3 uncovered lines are FFI defensive guard and Redis-dependent hot paths.
derive_tenant_keys() is a Rust FFI binding that never returns None — it either returns TenantKeys or raises an exception. The None check was dead code that couldn't be tested or triggered.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cachekit/serializers/encryption_wrapper.py (1)
164-166:⚠️ Potential issue | 🟡 MinorDocstring contains stale information about disabled encryption.
Line 166 states "Empty string allowed only when encryption is disabled," but encryption is now always enabled. This documentation is misleading since an empty
cache_keywill always raiseValueErrorper lines 212-216.📝 Proposed fix for docstring
cache_key: Cache key for AAD binding (SECURITY CRITICAL for encryption). Prevents ciphertext substitution attacks (Protocol v1.0.1, Section 5.6). - Empty string allowed only when encryption is disabled. + Must be non-empty (required for AAD binding).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/serializers/encryption_wrapper.py` around lines 164 - 166, The docstring for the cache_key parameter is stale and incorrectly says "Empty string allowed only when encryption is disabled" even though encryption is always enabled and the code (where cache_key is validated and a ValueError is raised) forbids empty values; update the parameter docstring in encryption_wrapper (the cache_key parameter description in the class/function that accepts cache_key) to remove that sentence and clearly state that an empty cache_key is not allowed and will raise ValueError (referencing the cache_key validation in the constructor/validation logic).
🧹 Nitpick comments (1)
src/cachekit/serializers/encryption_wrapper.py (1)
8-8: Documentation references outdated serializer name.The module docstring (line 8) and class docstring (line 45) still reference
AutoSerializer, but the default has been changed toStandardSerializerper line 103 and the actual implementation at lines 115-117.📝 Proposed fix to update docstrings
- It wraps any SerializerProtocol (AutoSerializer, OrjsonSerializer, ArrowSerializer) + It wraps any SerializerProtocol (StandardSerializer, OrjsonSerializer, ArrowSerializer)- - Works with ANY serializer (AutoSerializer, OrjsonSerializer, ArrowSerializer) + - Works with ANY serializer (StandardSerializer, OrjsonSerializer, ArrowSerializer)Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/serializers/encryption_wrapper.py` at line 8, Update the outdated docstrings in encryption_wrapper.py that reference AutoSerializer: change module-level docstring and the EncryptionWrapper class docstring (which currently mention AutoSerializer) to reference StandardSerializer as the default serializer and ensure any examples or text align with the actual default and implementation used by EncryptionWrapper and its __init__ default_serializer parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 164-166: The docstring for the cache_key parameter is stale and
incorrectly says "Empty string allowed only when encryption is disabled" even
though encryption is always enabled and the code (where cache_key is validated
and a ValueError is raised) forbids empty values; update the parameter docstring
in encryption_wrapper (the cache_key parameter description in the class/function
that accepts cache_key) to remove that sentence and clearly state that an empty
cache_key is not allowed and will raise ValueError (referencing the cache_key
validation in the constructor/validation logic).
---
Nitpick comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Line 8: Update the outdated docstrings in encryption_wrapper.py that reference
AutoSerializer: change module-level docstring and the EncryptionWrapper class
docstring (which currently mention AutoSerializer) to reference
StandardSerializer as the default serializer and ensure any examples or text
align with the actual default and implementation used by EncryptionWrapper and
its __init__ default_serializer parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fbbde04-66ff-4d69-b69a-dbea47f1a613
📒 Files selected for processing (1)
src/cachekit/serializers/encryption_wrapper.py
- Replace AutoSerializer references with StandardSerializer in module and class docstrings (AutoSerializer is not the default) - Remove incorrect "empty string allowed when encryption disabled" from cache_key param doc — encryption is always enabled, empty cache_key always raises ValueError
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 164-166: Move the cache_key validation to occur before delegating
to the wrapped serializer: in the encryption wrapper's serialize() method, check
that cache_key is a non-empty string and raise ValueError immediately (instead
of after calling self._wrapped.serialize), and do the same pre-check in
deserialize() (validate cache_key before calling self._wrapped.deserialize);
update the logic that references
self._wrapped.serialize/self._wrapped.deserialize and keep the same error
type/message to preserve behavior.
🪄 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
Run ID: 501e4aa9-7585-4b2e-946d-14e09775e6ce
📒 Files selected for processing (1)
src/cachekit/serializers/encryption_wrapper.py
Comment on lines
+164
to
+166
| cache_key: Cache key for AAD binding (SECURITY CRITICAL). | ||
| Prevents ciphertext substitution attacks (Protocol v1.0.1, Section 5.6). | ||
| Empty string allowed only when encryption is disabled. | ||
| Must be a non-empty string; raises ValueError if empty. |
There was a problem hiding this comment.
Validate cache_key before invoking the wrapped serializer.
serialize() currently serializes obj first, then validates cache_key. That can produce inconsistent error precedence and wasted work for invalid keys.
Proposed change
def serialize(self, obj: Any, cache_key: str = "") -> tuple[bytes, SerializationMetadata]:
@@
- # First serialize with base serializer
- try:
- raw_data, raw_metadata = self.serializer.serialize(obj)
- except Exception as e:
- raise SerializationError(f"Serialization failed: {e}") from e
-
# SECURITY: Validate cache_key type and value
if not isinstance(cache_key, str):
raise TypeError(
f"cache_key must be a string, got {type(cache_key).__name__}. "
"AAD v0x03 requires a string cache_key for ciphertext binding."
)
if not cache_key:
raise ValueError(
"cache_key is required when encryption is enabled. "
"AAD v0x03 requires cache_key binding to prevent ciphertext substitution attacks."
)
+
+ # First serialize with base serializer
+ try:
+ raw_data, raw_metadata = self.serializer.serialize(obj)
+ except Exception as e:
+ raise SerializationError(f"Serialization failed: {e}") from eAlso applies to: 206-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cachekit/serializers/encryption_wrapper.py` around lines 164 - 166, Move
the cache_key validation to occur before delegating to the wrapped serializer:
in the encryption wrapper's serialize() method, check that cache_key is a
non-empty string and raise ValueError immediately (instead of after calling
self._wrapped.serialize), and do the same pre-check in deserialize() (validate
cache_key before calling self._wrapped.deserialize); update the logic that
references self._wrapped.serialize/self._wrapped.deserialize and keep the same
error type/message to preserve behavior.
Move cache_key type/value validation before self.serializer.serialize() in EncryptionWrapper.serialize(). Fail fast on invalid input instead of wasting work serializing data that can't be encrypted. Also fix stale deserialize docstring claiming empty cache_key is allowed for unencrypted data.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cachekit/serializers/encryption_wrapper.py (1)
164-166: Cache-key prevalidation is correctly moved ahead of serialization.This fixes the AAD v0x03 fail-fast behavior and keeps exception semantics clean.
Optional: consider extracting shared cache_key validation into a private helper to avoid drift between serialize/deserialize paths.Also applies to: 200-200, 212-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cachekit/serializers/encryption_wrapper.py` around lines 164 - 166, The cache_key prevalidation was duplicated and should be consolidated to avoid divergence between serialize and deserialize; add a private helper (e.g., _validate_cache_key or validate_cache_key) inside the EncryptionWrapper module/class that checks cache_key is a non-empty string and raises ValueError if empty, then call this helper from both serialize and deserialize (and any other paths noted around lines ~164 and ~200/212-216) so both paths share the same logic and fail-fast behavior for AAD v0x03 remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cachekit/serializers/encryption_wrapper.py`:
- Around line 164-166: The cache_key prevalidation was duplicated and should be
consolidated to avoid divergence between serialize and deserialize; add a
private helper (e.g., _validate_cache_key or validate_cache_key) inside the
EncryptionWrapper module/class that checks cache_key is a non-empty string and
raises ValueError if empty, then call this helper from both serialize and
deserialize (and any other paths noted around lines ~164 and ~200/212-216) so
both paths share the same logic and fail-fast behavior for AAD v0x03 remains
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dcb4707-9d50-4f78-a3e0-6b4af7855de1
📒 Files selected for processing (1)
src/cachekit/serializers/encryption_wrapper.py
CRITICAL: Two async cache-write branches in wrapper.py (lines 1142,
1219) were calling serialize_data() without cache_key. With encryption
enabled, this would fail AAD v0x03 binding. Sync paths already had it.
Canonicalize serializer aliases ("std", "standard") to "default" in
CacheSerializationHandler._serializer_string_name to prevent envelope
mismatch when deserializing data written with a different alias.
Fix docs/serializers/encryption.md: AutoSerializer -> StandardSerializer
as the default inner serializer for EncryptionWrapper.
PR CI was only running tests/critical/, missing tests/unit/ entirely. This meant codecov patch coverage couldn't reflect unit test coverage for new code. Unit tests are fast (not slow-marked) so there's no reason to exclude them from PR runs.
test_encryption_config_validation.py and test_reliability_fallback.py have pre-existing failures due to settings singleton leakage between tests (missing reset_settings() calls). These were previously hidden because PR CI only ran tests/critical/. Excluding them until they're fixed separately — they still run on push to main via the full suite.
Tests were failing because validate_encryption_config() reads from the pydantic-settings singleton, which caches across tests. monkeypatch.setenv changes the env var but the singleton still holds stale values. Added autouse fixture that resets the singleton before and after each test. All 41 tests now pass. Also removed the --ignore for this file from CI (only keeping the ignore for test_reliability_fallback.py which genuinely needs Redis).
Tests were integration tests masquerading as unit tests — they called get_redis_client() and get_health_checker() which require a live Redis connection and registered DI container. Fixed by: - Removing redis_client fixture (no real connections in unit tests) - Adding backend=None to @cache decorators (L1-only mode) - Constructing HealthChecker directly with mocked container/backend - Adding test_health_check_redis_unhealthy for the failure path Removed --ignore from CI — all unit tests now pass without Redis.
… test 3 backend resolution tests were calling _resolve_backend() which hits the DI container for CacheClientProvider — fails without Redis. Fixed by patching DIContainer.get to return a mock. Deleted test_cache_info_deprecated_record_hit — tests record_hit() which was removed in this PR's dead code cleanup. Test was testing a method that no longer exists.
9 tasks
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
1 participant
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.
Summary
deserialize_data()calls missingcache_keyfor AAD v0x03 verification — encrypted L1+L2 and L2 cache hits would fail AAD verification silentlyAAD Bug (fix: commit)
All
deserialize_data()calls in the L1+L2 and L2 paths were missingcache_key=cache_key, causing AAD verification failure for encrypted caches. L1-only paths already passed it correctly. The bug meant encrypted cache hits would fail silently and fall through to cache miss on every request.Dead Code Removed
setup_distributed_tracinglogging.pysetup_correlation_trackinglogging.pyreset_session_iddecorators/session.pyget_async_http_clientbackends/cachekitio/client.pyrecord_cache_operation(module)reliability/metrics_collection.pyrecord_circuit_breaker_state(module)reliability/metrics_collection.pyrecord_timeout_adjustment(module)reliability/metrics_collection.py_use_pipelined(variable)decorators/wrapper.pyuse_pipelined(variable)decorators/wrapper.pylock_key(sync only)decorators/wrapper.pyTest plan
__all__exportsclose_async_clientthread-local cleanup preserved (SEC-001)get_session_iddoctests updatedSummary by CodeRabbit
Deprecated API Removals
Chores / Behavior Changes
safe_modeconfiguration removed; encryption is now mandatory with stricter validation and tenant handling; serializer surface consolidated around "StandardSerializer".Documentation
Tests