LCORE-605: remove assert from production code #461
Merged
Conversation
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
Contributor
WalkthroughAdds a new public exception Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant AppConfig
note over AppConfig: Inline loaded-state guards (no new helper)
Caller->>AppConfig: access property (e.g., configuration, inference)
alt configuration not loaded
AppConfig-->>Caller: raise LogicError("logic error: configuration is not loaded")
else configuration loaded
AppConfig-->>Caller: return requested property
end
sequenceDiagram
autonumber
actor Caller
participant AppConfig
note over AppConfig: authorization returns default when absent
Caller->>AppConfig: authorization_configuration
AppConfig->>AppConfig: check inline authorization value
alt authorization is None
AppConfig-->>Caller: return AuthorizationConfiguration()
else authorization present
AppConfig-->>Caller: return configured authorization
end
sequenceDiagram
autonumber
actor Caller
participant AuthCfg as AuthenticationConfiguration
note over AuthCfg: explicit JWK validation
Caller->>AuthCfg: jwk_configuration()
alt jwk_config is None
AuthCfg-->>Caller: raise ValueError("JWK configuration should not be None")
else jwk_config present
AuthCfg-->>Caller: return jwk_config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
force-pushed
the
lcore-605-remove-assert-from-production-code
branch
from
dfc7e52 to
c4a8c83
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/configuration.py (2)
47-54: Do not log full configuration; add error handling and redact secrets.Logging the entire config at info level risks leaking API keys/tokens; also no handling for missing/invalid YAML.
Apply:
def load_configuration(self, filename: str) -> None: """Load configuration from YAML file.""" - with open(filename, encoding="utf-8") as fin: - config_dict = yaml.safe_load(fin) - config_dict = replace_env_vars(config_dict) - logger.info("Loaded configuration: %s", config_dict) - self.init_from_dict(config_dict) + try: + with open(filename, encoding="utf-8") as fin: + loaded = yaml.safe_load(fin) + except FileNotFoundError as exc: + raise FileNotFoundError(f"Configuration file not found: {filename}") from exc + except yaml.YAMLError as exc: + raise ValueError(f"Invalid YAML in configuration file: {filename}") from exc + if not isinstance(loaded, dict): + raise TypeError("Configuration root must be a mapping (YAML object).") + config_dict = replace_env_vars(loaded) + logger.debug("Loaded configuration keys: %s", list(config_dict.keys())) + self.init_from_dict(config_dict)
55-58: Validate input to prevent runtime type errors.def init_from_dict(self, config_dict: dict[Any, Any]) -> None: """Initialize configuration from a dictionary.""" - self._configuration = Configuration(**config_dict) + if not isinstance(config_dict, dict): + raise TypeError("init_from_dict expects a dictionary mapping.") + self._configuration = Configuration(**config_dict)
🧹 Nitpick comments (3)
src/configuration.py (3)
62-63: DRY the loaded-state guard by delegating toconfiguration.Leverage the
configurationproperty’s guard to eliminate repetition.def service_configuration(self) -> ServiceConfiguration: """Return service configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.service + return self.configuration.service def llama_stack_configuration(self) -> LlamaStackConfiguration: """Return Llama stack configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.llama_stack + return self.configuration.llama_stack def user_data_collection_configuration(self) -> UserDataCollection: """Return user data collection configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.user_data_collection + return self.configuration.user_data_collection def mcp_servers(self) -> list[ModelContextProtocolServer]: """Return model context protocol servers configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.mcp_servers + return self.configuration.mcp_servers def authentication_configuration(self) -> AuthenticationConfiguration: """Return authentication configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - - if self._configuration.authentication is None: - raise LogicError("logic error: authentication configuration is not loaded") - - return self._configuration.authentication + _cfg = self.configuration + if _cfg.authentication is None: + raise LogicError("logic error: authentication configuration is not loaded") + return _cfg.authentication def authorization_configuration(self) -> AuthorizationConfiguration: """Return authorization configuration or default no-op configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - - if self._configuration.authorization is None: - return AuthorizationConfiguration() - - return self._configuration.authorization + _cfg = self.configuration + return _cfg.authorization or AuthorizationConfiguration() def customization(self) -> Optional[Customization]: """Return customization configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.customization + return self.configuration.customization def inference(self) -> InferenceConfiguration: """Return inference configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.inference + return self.configuration.inference def database_configuration(self) -> DatabaseConfiguration: """Return database configuration.""" - if self._configuration is None: - raise LogicError("logic error: configuration is not loaded") - return self._configuration.database + return self.configuration.databaseAlso applies to: 69-71, 76-78, 83-85, 90-92, 97-99, 108-110, 119-121, 126-128, 133-135
108-114: Avoid returning a fresh default Authorization object on every access.Callers mutating
access_ruleswon’t see persistence between calls. Cache a singleton default or document immutability.Within this property (if you choose to cache):
- _cfg = self.configuration - return _cfg.authorization or AuthorizationConfiguration() + _cfg = self.configuration + return _cfg.authorization or self._default_authorizationAnd initialize once in
__init__(outside this range):# in __init__ self._default_authorization = AuthorizationConfiguration()
28-30: Prefer deriving from RuntimeError; consider centralizing exceptions.Minor: deriving from RuntimeError is semantically closer for program-logic faults. Optionally move to a shared errors module if reused.
-class LogicError(Exception): +class LogicError(RuntimeError): """Error in application logic."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/configuration.py(3 hunks)tests/unit/test_configuration.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_configuration.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/configuration.py (1)
src/models/config.py (8)
ServiceConfiguration(129-146)LlamaStackConfiguration(157-200)UserDataCollection(203-220)ModelContextProtocolServer(149-154)AuthenticationConfiguration(341-377)AuthorizationConfiguration(316-321)InferenceConfiguration(398-415)DatabaseConfiguration(88-126)
⏰ 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). (1)
- GitHub Check: build-pr
🔇 Additional comments (1)
src/configuration.py (1)
100-102: LGTM: Raising LogicError for missing authentication.Matches the PR goal to remove asserts and surface a domain exception.
Comment on lines
+28
to
+30
| class LogicError(Exception): | ||
| """Error in application logic.""" | ||
|
|
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify no remaining assert in production code (repo-wide).
🏁 Script executed:
#!/bin/bash
# Find asserts outside tests and docs.
rg -nP --type=python -g '!**/tests/**' -g '!**/doc/**' -C2 '^\s*assert\b'Length of output: 106
Verify no remaining assert in production code (repo-wide).
🏁 Script executed:
#!/bin/bash
# Find 'assert' statements outside tests and docs.
rg -nP -g '*.py' -g '!**/tests/**' -g '!**/doc/**' -g '!**/docs/**' -C2 '^\s*assert\b'Length of output: 1892
Remove all assert statements from production code
- scripts/generate_openapi_schema.py (lines 31–35, 86–88): replace each
assertwith explicit validation and raise a specific exception (e.g.ValueErroror a custom error). - src/app/database.py (lines 118, 122): replace type-check
assertwith runtime checks that raiseTypeErroror a domain-specific exception.
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.
Description
LCORE-605: remove
assertfrom production codeType of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Bug Fixes