LCORE-298: Conversation cache factory #567
Conversation
WalkthroughAdds a new Cache abstraction and factory, renames ConversationCache → ConversationCacheConfiguration, updates Configuration and AppConfig accessors (adds a lazy Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant AppConfig
participant Configuration
participant CacheFactory
participant Cache as CacheImpl
Note over AppConfig,Configuration: configuration accessor (unchanged backing field)
Caller->>AppConfig: get conversation_cache_configuration
AppConfig->>Configuration: access conversation_cache
Configuration-->>AppConfig: ConversationCacheConfiguration
AppConfig-->>Caller: ConversationCacheConfiguration
Note over AppConfig,CacheFactory: lazy cache construction path
Caller->>AppConfig: get conversation_cache
alt cache cached
AppConfig-->>Caller: return cached Cache
else not cached
AppConfig->>CacheFactory: conversation_cache(ConversationCacheConfiguration)
CacheFactory-->>AppConfig: Cache | None
AppConfig->>AppConfig: store in _conversation_cache
AppConfig-->>Caller: return Cache | None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tisnik
force-pushed
the
lcore-298-conversation-cache-factory-
branch
2 times, most recently
from
0a7a605 to
1d6107c
Compare
tisnik
force-pushed
the
lcore-298-conversation-cache-factory-
branch
from
1d6107c to
15d0379
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/configuration.py (2)
57-57: Do not log full configuration payload (secrets/PII risk)This can leak credentials/URLs into logs.
- logger.info("Loaded configuration: %s", config_dict) + logger.info("Loaded configuration from %s", filename)If you need visibility, add a redaction helper and log only non‑sensitive sections.
133-138: Return type must be Optional to match modelconfiguration.conversation_cache is Optional[…]; current annotation is incorrect and can mislead callers.
- def conversation_cache_configuration(self) -> ConversationCacheConfiguration: - """Return conversation cache configuration.""" + def conversation_cache_configuration(self) -> Optional[ConversationCacheConfiguration]: + """Return conversation cache configuration if configured, otherwise None.""" if self._configuration is None: raise LogicError("logic error: configuration is not loaded") return self._configuration.conversation_cache
🧹 Nitpick comments (5)
src/models/cache_entry.py (1)
6-19: Consider forbidding unknown fields and adding field descriptionsTighten the model and improve generated schema/docs.
Apply:
-from pydantic import BaseModel +from pydantic import BaseModel, Field +from pydantic import ConfigDict @@ class CacheEntry(BaseModel): @@ - query: str - response: str - provider: str - model: str + # Reject unexpected keys + model_config = ConfigDict(extra="forbid") + + query: str = Field(..., description="User query text") + response: str = Field(..., description="Model response text") + provider: str = Field(..., description="Provider identifier (e.g., 'openai')") + model: str = Field(..., description="Model identifier/version")src/cache/cache_factory.py (1)
3-9: Use standard logger per repo guidelinesAlign with “logger = logging.getLogger(name)” and drop custom get_logger here for consistency with src/configuration.py.
-import constants -from models.config import ConversationCacheConfiguration -from cache.cache import Cache -from log import get_logger +import logging +import constants +from models.config import ConversationCacheConfiguration +from cache.cache import Cache @@ -logger = get_logger("cache.cache_factory") +logger = logging.getLogger(__name__)src/cache/cache.py (2)
19-21: Annotate class constant as ClassVar[str]Matches guideline “type annotations for all class attributes.”
-from abc import ABC, abstractmethod +from abc import ABC, abstractmethod +from typing import ClassVar @@ - COMPOUND_KEY_SEPARATOR = ":" + COMPOUND_KEY_SEPARATOR: ClassVar[str] = ":"
56-58: Fix return contract in docstringType is list[CacheEntry], so don’t mention None; prefer empty list when not found.
- Returns: - The value (CacheEntry(s)) associated with the key, or None if not found. + Returns: + A list of CacheEntry associated with the key (empty list if not found).src/configuration.py (1)
11-23: Inconsistent logger usage — standardize on one approachRepo mixes the custom get_logger (src/log.py) and direct logging.getLogger(...) calls. Examples: src/cache/cache_factory.py uses get_logger(...); src/configuration.py uses logging.getLogger(name). Choose one approach and apply repo‑wide. If you keep the helper, stop changing logger state in the helper (remove per‑module setLevel) and centralize logging configuration in the app entrypoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/config.pngis excluded by!**/*.pngdocs/config.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
docs/config.puml(3 hunks)src/cache/__init__.py(1 hunks)src/cache/cache.py(1 hunks)src/cache/cache_error.py(1 hunks)src/cache/cache_factory.py(1 hunks)src/configuration.py(4 hunks)src/models/cache_entry.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/cache/init.py
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/cache/cache_error.pysrc/models/cache_entry.pysrc/cache/cache_factory.pysrc/cache/cache.pysrc/configuration.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/cache/cache_error.pysrc/models/cache_entry.pysrc/cache/cache_factory.pysrc/cache/cache.pysrc/configuration.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/cache_entry.pysrc/configuration.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/cache_entry.py
src/{models/config.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields
Files:
src/configuration.py
🧠 Learnings (3)
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/{models/config.py,configuration.py} : All configuration uses Pydantic models extending ConfigurationBase
Applied to files:
docs/config.puml
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/models/**/*.py : Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Applied to files:
docs/config.puml
📚 Learning: 2025-09-18T16:46:33.353Z
Learnt from: CR
PR: lightspeed-core/lightspeed-stack#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T16:46:33.353Z
Learning: Applies to src/{models/config.py,configuration.py} : Configuration base models must set model_config with extra="forbid" to reject unknown fields
Applied to files:
docs/config.puml
🧬 Code graph analysis (3)
src/cache/cache_factory.py (4)
src/models/config.py (2)
config(138-144)ConversationCacheConfiguration(489-527)src/cache/cache.py (1)
Cache(9-110)src/log.py (1)
get_logger(7-13)src/configuration.py (1)
conversation_cache(147-153)
src/cache/cache.py (2)
src/models/cache_entry.py (1)
CacheEntry(6-19)src/utils/suid.py (1)
check_suid(15-28)
src/configuration.py (3)
src/models/config.py (1)
ConversationCacheConfiguration(489-527)src/cache/cache.py (1)
Cache(9-110)src/cache/cache_factory.py (2)
CacheFactory(12-35)conversation_cache(16-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (4)
src/cache/cache_error.py (1)
1-5: LGTM — dedicated CacheError typeClear, minimal, and fits the new cache layer.
docs/config.puml (1)
32-32: Diagram updates look correctReferences and inheritance updated to ConversationCacheConfiguration and optionality reflected.
Also applies to: 46-52, 160-161
src/configuration.py (1)
132-138: No call‑site breakage detected — rename is safe.
Tests and callers use conversation_cache_configuration for the config object; runtime cache remains conversation_cache (CacheFactory/internal _conversation_cache).src/cache/cache.py (1)
92-102: Avoid method name shadowing built‑in list() — rename to list_conversationsRename the abstract method in src/cache/cache.py (lines 92–102) to list_conversations to avoid shadowing Python's built-in and clarify intent. The provided ripgrep output shows only client.agents.session.list calls and no references to Cache.list, so rename before first implementation to prevent API churn.
| @staticmethod | ||
| def conversation_cache(config: ConversationCacheConfiguration) -> Cache | None: | ||
| """Create an instance of Cache based on loaded configuration. | ||
| Returns: | ||
| An instance of `Cache` (either `PostgresCache` or `InMemoryCache`). | ||
| """ | ||
| logger.info("Creating cache instance of type %s", config.type) | ||
| match config.type: | ||
| case constants.CACHE_TYPE_MEMORY: | ||
| return None | ||
| case constants.CACHE_TYPE_SQLITE: | ||
| return None | ||
| case constants.CACHE_TYPE_POSTGRES: | ||
| return None | ||
| case _: | ||
| raise ValueError( | ||
| f"Invalid cache type: {config.type}. " | ||
| f"Use '{constants.CACHE_TYPE_POSTGRES}' '{constants.CACHE_TYPE_SQLITE}' or " | ||
| f"'{constants.CACHE_TYPE_MEMORY}' options." | ||
| ) |
There was a problem hiding this comment.
Handle None/disabled cache config and fix docstring/options text
Factory currently assumes config is non‑None and will crash when configuration.conversation_cache is None or when type is None (a valid state per validator). Also docstring mentions only Postgres/InMemory while SQLite is supported by the type.
class CacheFactory:
@@
- def conversation_cache(config: ConversationCacheConfiguration) -> Cache | None:
- """Create an instance of Cache based on loaded configuration.
-
- Returns:
- An instance of `Cache` (either `PostgresCache` or `InMemoryCache`).
- """
- logger.info("Creating cache instance of type %s", config.type)
- match config.type:
+ def conversation_cache(config: ConversationCacheConfiguration | None) -> Cache | None:
+ """Create an instance of Cache based on loaded configuration.
+
+ Returns:
+ A Cache instance, or None when cache is not configured/disabled or not yet implemented.
+ """
+ if config is None or config.type is None:
+ logger.info("Conversation cache disabled or not configured")
+ return None
+
+ logger.info("Creating cache instance of type %s", config.type)
+ match config.type:
case constants.CACHE_TYPE_MEMORY:
return None
case constants.CACHE_TYPE_SQLITE:
return None
case constants.CACHE_TYPE_POSTGRES:
return None
case _:
raise ValueError(
- f"Invalid cache type: {config.type}. "
- f"Use '{constants.CACHE_TYPE_POSTGRES}' '{constants.CACHE_TYPE_SQLITE}' or "
- f"'{constants.CACHE_TYPE_MEMORY}' options."
+ f"Invalid cache type: {config.type}. "
+ f"Use '{constants.CACHE_TYPE_POSTGRES}', '{constants.CACHE_TYPE_SQLITE}', or "
+ f"'{constants.CACHE_TYPE_MEMORY}'."
)📝 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.
| @staticmethod | |
| def conversation_cache(config: ConversationCacheConfiguration) -> Cache | None: | |
| """Create an instance of Cache based on loaded configuration. | |
| Returns: | |
| An instance of `Cache` (either `PostgresCache` or `InMemoryCache`). | |
| """ | |
| logger.info("Creating cache instance of type %s", config.type) | |
| match config.type: | |
| case constants.CACHE_TYPE_MEMORY: | |
| return None | |
| case constants.CACHE_TYPE_SQLITE: | |
| return None | |
| case constants.CACHE_TYPE_POSTGRES: | |
| return None | |
| case _: | |
| raise ValueError( | |
| f"Invalid cache type: {config.type}. " | |
| f"Use '{constants.CACHE_TYPE_POSTGRES}' '{constants.CACHE_TYPE_SQLITE}' or " | |
| f"'{constants.CACHE_TYPE_MEMORY}' options." | |
| ) | |
| @staticmethod | |
| def conversation_cache(config: ConversationCacheConfiguration | None) -> Cache | None: | |
| """Create an instance of Cache based on loaded configuration. | |
| Returns: | |
| A Cache instance, or None when cache is not configured/disabled or not yet implemented. | |
| """ | |
| if config is None or config.type is None: | |
| logger.info("Conversation cache disabled or not configured") | |
| return None | |
| logger.info("Creating cache instance of type %s", config.type) | |
| match config.type: | |
| case constants.CACHE_TYPE_MEMORY: | |
| return None | |
| case constants.CACHE_TYPE_SQLITE: | |
| return None | |
| case constants.CACHE_TYPE_POSTGRES: | |
| return None | |
| case _: | |
| raise ValueError( | |
| f"Invalid cache type: {config.type}. " | |
| f"Use '{constants.CACHE_TYPE_POSTGRES}', '{constants.CACHE_TYPE_SQLITE}', or " | |
| f"'{constants.CACHE_TYPE_MEMORY}'." | |
| ) |
🤖 Prompt for AI Agents
In src/cache/cache_factory.py around lines 15 to 35, the factory assumes a
non-None config and a non-None config.type and the docstring only lists
Postgres/InMemory; change the function to first handle a None/disabled config by
returning None if config is None or config.type is falsy, then use the existing
match on config.type to return the appropriate Cache implementations (or None
for in-memory/disabled), and update the docstring and the thrown ValueError
message to list all supported options including SQLite, Postgres and InMemory so
the documentation and error text match actual supported types.
| @property | ||
| def conversation_cache(self) -> Cache | None: | ||
| """Return the conversation cache.""" | ||
| if self._conversation_cache is None and self._configuration is not None: | ||
| self._conversation_cache = CacheFactory.conversation_cache( | ||
| self._configuration.conversation_cache | ||
| ) | ||
| return self._conversation_cache |
There was a problem hiding this comment.
Guard for unloaded config and None cache config before calling factory
Avoids passing None to the factory and keeps behavior consistent with other accessors.
@property
def conversation_cache(self) -> Cache | None:
"""Return the conversation cache."""
- if self._conversation_cache is None and self._configuration is not None:
- self._conversation_cache = CacheFactory.conversation_cache(
- self._configuration.conversation_cache
- )
- return self._conversation_cache
+ if self._configuration is None:
+ raise LogicError("logic error: configuration is not loaded")
+ if self._conversation_cache is None:
+ cfg = self._configuration.conversation_cache
+ if cfg is None or cfg.type is None:
+ self._conversation_cache = None
+ else:
+ self._conversation_cache = CacheFactory.conversation_cache(cfg)
+ return self._conversation_cache📝 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.
| @property | |
| def conversation_cache(self) -> Cache | None: | |
| """Return the conversation cache.""" | |
| if self._conversation_cache is None and self._configuration is not None: | |
| self._conversation_cache = CacheFactory.conversation_cache( | |
| self._configuration.conversation_cache | |
| ) | |
| return self._conversation_cache | |
| @property | |
| def conversation_cache(self) -> Cache | None: | |
| """Return the conversation cache.""" | |
| if self._configuration is None: | |
| raise LogicError("logic error: configuration is not loaded") | |
| if self._conversation_cache is None: | |
| cfg = self._configuration.conversation_cache | |
| if cfg is None or cfg.type is None: | |
| self._conversation_cache = None | |
| else: | |
| self._conversation_cache = CacheFactory.conversation_cache(cfg) | |
| return self._conversation_cache |
🤖 Prompt for AI Agents
In src/configuration.py around lines 146 to 153, the property currently calls
CacheFactory.conversation_cache even when self._configuration is None or when
self._configuration.conversation_cache is None; add a guard so the factory is
only called if self._conversation_cache is None AND self._configuration is not
None AND self._configuration.conversation_cache is not None, otherwise leave
self._conversation_cache as None and return it; mirror the same defensive
pattern used by the other accessors to avoid passing None into the factory.
1 participant
Description
LCORE-298: Conversation cache factory
Type of change
Related Tickets & Documents
Summary by CodeRabbit
Refactor
New Features
Tests
Documentation