LCORE-390: authentication configuration is not optional#395
Conversation
WalkthroughThe Configuration model’s authentication field was made non-optional with a default AuthenticationConfiguration. A new unit test validates default and explicit authentication configurations, including module selection, TLS verification flag, and certificate path behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ 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 (
|
![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
🧹 Nitpick comments (3)
src/models/config.py (2)
108-108: Optional: use default_factory for other nested models to avoid shared instancesFor consistency and to avoid any risk of shared defaults, prefer default factories for nested models as well.
Apply these diffs:
- tls_config: TLSConfiguration = TLSConfiguration() + tls_config: TLSConfiguration = Field(default_factory=TLSConfiguration)- database: DatabaseConfiguration = DatabaseConfiguration() + database: DatabaseConfiguration = Field(default_factory=DatabaseConfiguration)- inference: InferenceConfiguration = InferenceConfiguration() + inference: InferenceConfiguration = Field(default_factory=InferenceConfiguration)Note: Ensure Field is imported from pydantic:
from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, FieldAlso applies to: 293-293, 297-297
241-245: Avoid assert for runtime validation; raise ValueError insteadUsing assert for invariant checks can be disabled with Python optimizations and results in AssertionError, not a validation error. Raise a ValueError to match the surrounding validation semantics.
- assert self.jwk_config is not None, "JWK configuration should not be None" - return self.jwk_config + if self.jwk_config is None: + raise ValueError("JWK configuration should not be None") + return self.jwk_configtests/unit/models/test_config.py (1)
617-663: Add tests to guard against explicit None and shared-default regressionsTwo focused tests would harden the change:
- Assert that explicitly passing authentication=None raises ValidationError.
- Assert each Configuration gets a unique AuthenticationConfiguration instance (no shared default).
You can add:
def test_configuration_authentication_explicit_none_rejected() -> None: from pydantic import ValidationError with pytest.raises(ValidationError): Configuration( name="test_name", service=ServiceConfiguration(), llama_stack=LlamaStackConfiguration( use_as_library_client=True, library_client_config_path="tests/configuration/run.yaml", ), user_data_collection=UserDataCollection( feedback_enabled=False, feedback_storage=None ), authentication=None, # explicit None should be rejected ) def test_configuration_authentication_default_is_per_instance() -> None: cfg_a = Configuration( name="a", service=ServiceConfiguration(), llama_stack=LlamaStackConfiguration( use_as_library_client=True, library_client_config_path="tests/configuration/run.yaml", ), user_data_collection=UserDataCollection( feedback_enabled=False, feedback_storage=None ), ) cfg_b = Configuration( name="b", service=ServiceConfiguration(), llama_stack=LlamaStackConfiguration( use_as_library_client=True, library_client_config_path="tests/configuration/run.yaml", ), user_data_collection=UserDataCollection( feedback_enabled=False, feedback_storage=None ), ) # ensure a fresh default per instance assert cfg_a.authentication is not cfg_b.authentication # mutate one and ensure it doesn't affect the other cfg_a.authentication.skip_tls_verification = True assert cfg_b.authentication.skip_tls_verification is FalseI can open a follow-up PR with these tests if you prefer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/models/config.py(1 hunks)tests/unit/models/test_config.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (5)
Configuration(286-302)ServiceConfiguration(99-119)LlamaStackConfiguration(130-173)UserDataCollection(176-193)AuthenticationConfiguration(210-245)
⏰ 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: e2e_tests
🔇 Additional comments (1)
tests/unit/models/test_config.py (1)
617-663: LGTM: Solid coverage for default and explicit authentication configurationThe test verifies the non-optional default behavior and preserves explicit settings, including type coercion for FilePath. Looks good.
| @@ -292,9 +292,7 @@ class Configuration(BaseModel): | |||
| user_data_collection: UserDataCollection | |||
| database: DatabaseConfiguration = DatabaseConfiguration() | |||
| mcp_servers: list[ModelContextProtocolServer] = [] | |||
There was a problem hiding this comment.
Fix mutable default list for mcp_servers
mcp_servers uses a mutable default ([]), which will be shared across Configuration instances. Use a default factory to prevent cross-instance leakage.
Apply this diff:
-mcp_servers: list[ModelContextProtocolServer] = []
+mcp_servers: list[ModelContextProtocolServer] = Field(default_factory=list)📝 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.
| mcp_servers: list[ModelContextProtocolServer] = [] | |
| mcp_servers: list[ModelContextProtocolServer] = Field(default_factory=list) |
🤖 Prompt for AI Agents
In src/models/config.py around line 294, mcp_servers is defined with a mutable
default list ([]), which will be shared across instances; change the definition
to use a default factory instead (e.g., if this is a dataclass use
field(default_factory=list) from dataclasses, or if this is a Pydantic model use
Field(default_factory=list) from pydantic) so each Configuration instance gets
its own new list; update the import if needed and remove the bare [] default.
| authentication: Optional[AuthenticationConfiguration] = ( | ||
| AuthenticationConfiguration() | ||
| ) | ||
| authentication: AuthenticationConfiguration = AuthenticationConfiguration() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid shared default instance for authentication; use Field(default_factory=...)
Using a constructed BaseModel instance as a class-level default risks sharing the same instance across all Configuration objects. Prefer a factory to ensure a fresh instance per model.
Apply this diff to the authentication field:
-authentication: AuthenticationConfiguration = AuthenticationConfiguration()
+authentication: AuthenticationConfiguration = Field(default_factory=AuthenticationConfiguration)Additionally, ensure Field is imported:
# at import line with other pydantic symbols
from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, FieldThis change aligns the code with the PR’s goal (authentication is not optional) but may uncover existing configs passing authentication: null. Run this to find potential offenders:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching YAML for 'authentication: null' or '~' or empty mapping lines"
rg -n --glob '*.yaml' --glob '*.yml' -e $'^\s*authentication\s*:\s*(null|~)\s*$' -e $'^\s*authentication\s*:\s*$' || true
echo "Searching Python for explicit authentication=None"
rg -n --type py -e $'authentication\s*=\s*None' -A 2 || trueLength of output: 634
Use a fresh AuthenticationConfiguration via default_factory
Sharing a single AuthenticationConfiguration() across all Configuration instances risks cross-instance state. Update the field and imports as follows, and adjust your example YAMLs to remove any explicit authentication: entries (which currently parse as null and will now error):
-
In src/models/config.py, replace:
- authentication: AuthenticationConfiguration = AuthenticationConfiguration() + authentication: AuthenticationConfiguration = Field(default_factory=AuthenticationConfiguration)
-
In the same file’s imports, add
Field:- from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl + from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, Field
-
Remove or replace the blank/null
authentication:lines in your YAML examples so they don’t explicitly setnull:• examples/lightspeed-stack.yaml:24
• examples/lightspeed-stack-lls-external.yaml:26
• examples/lightspeed-stack-lls-library.yaml:25You can either delete those lines (letting the default factory apply) or set them to an empty mapping (
authentication: {}) if you need an explicit no-overrides placeholder.
📝 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.
| authentication: AuthenticationConfiguration = AuthenticationConfiguration() | |
| # In src/models/config.py | |
| from pydantic import BaseModel, model_validator, FilePath, AnyHttpUrl, Field | |
| class Configuration(BaseModel): | |
| # … | |
| authentication: AuthenticationConfiguration = Field(default_factory=AuthenticationConfiguration) | |
| # … |
1 participant
Description
LCORE-390: authentication configuration is not optional
Type of change
Related Tickets & Documents
Summary by CodeRabbit