Skip to content

LCORE-973: configuration options descriptions#854

Merged
tisnik merged 11 commits intolightspeed-core:mainfrom
tisnik:lcore-973-configuration-description
Nov 30, 2025
Merged

LCORE-973: configuration options descriptions#854
tisnik merged 11 commits intolightspeed-core:mainfrom
tisnik:lcore-973-configuration-description

Conversation

@tisnik
Copy link
Contributor

@tisnik tisnik commented Nov 30, 2025

Description

LCORE-973: configuration options descriptions

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: CodeRabbit
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-973

Summary by CodeRabbit

  • New Features

    • Richer, user-facing configuration fields for service, cache, protocol server, Llama Stack, JWT role rules, conversation history, quota limiters/scheduler and handlers to improve setup and UI presentation.
  • Bug Fixes

    • Added validations to catch invalid ports, JWT rule issues (jsonpath/regex/roles), Llama Stack mode/URL mismatches, quota configuration errors, and related config problems earlier.
  • Documentation

    • Expanded titles, descriptions, and docstrings across configuration schemas and generated docs for clearer guidance and UI help.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 30, 2025

Walkthrough

Added Pydantic Field metadata, richer docstrings, and extended validation across many configuration models in src/models/config.py (ServiceConfiguration, InMemoryCacheConfig, ModelContextProtocolServer, LlamaStackConfiguration, JwtRoleRule, QuotaLimiterConfiguration, TLS/CORS, conversation history). Documentation files updated to reflect schema and textual changes.

Changes

Cohort / File(s) Summary
Primary config file
src/models/config.py
Converted many plain annotations to Field(..., title=..., description=...), expanded docstrings, added/updated validation (e.g., ServiceConfiguration.check_service_configuration port bounds; LlamaStack library vs server checks; JwtRoleRule JSONPath/operator/regex validation and role checks), added quota limiter/scheduler fields, removed mcp_servers from public surface.
Generated docs (HTML/JSON/MD)
docs/config.html, docs/config.json, docs/config.md
Regenerated documentation to include new titles/descriptions, nullability adjustments in schemas, expanded explanatory content and examples for LlamaStack, MCP, TLS/CORS, quota handlers/limiters/scheduler, database and user data collection; textual renames (e.g., Conversation cache → Conversation history).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas to review closely:
    • ServiceConfiguration.check_service_configuration (port bounds and return behavior).
    • JwtRoleRule validation: JSONPath validation, MATCH regex compilation and error handling, wildcard/role uniqueness checks.
    • LlamaStackConfiguration validation for library vs server modes and library_client_config_path presence/validation.
    • QuotaLimiterConfiguration and QuotaSchedulerConfiguration new fields and any serialization/schema implications.

Possibly related PRs

Suggested labels

ok-to-test

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change—adding and revising descriptions for configuration options across the codebase, which is supported by extensive updates to config.py, config.html, config.json, and config.md.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6cfaeab and ac99077.

📒 Files selected for processing (1)
  • src/models/config.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
⏰ 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). (4)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/models/config.py (1)

286-317: Optional: tighten wording in ServiceConfiguration docstring and field descriptions

Purely editorial, but you could make the docs a bit smoother and slightly more explicit about workers:

  • Add missing articles (“a specified hostname”, “the number of Uvicorn workers”).
  • Clarify that workers are Uvicorn workers in the field description.

Suggested diff:

 class ServiceConfiguration(ConfigurationBase):
-    """Service configuration.
-
-    Lightspeed Core Stack is a REST API service that needs to accept requests
-    on specified hostname and port. It is also possible to enable
-    authentication and specify number of Uvicorn workers. When more workers are
-    specified, it will be possible to serve requests concurrently.
-    """
+    """Service configuration.
+
+    Lightspeed Core Stack is a REST API service that accepts requests
+    on a specified hostname and port. It is also possible to enable
+    authentication and specify the number of Uvicorn workers. When more
+    workers are specified, the service can handle requests concurrently.
+    """

@@
-    auth_enabled: bool = Field(
-        False,
-        title="Authentication enabled",
-        description="Enables authentication subsystem",
-    )
+    auth_enabled: bool = Field(
+        False,
+        title="Authentication enabled",
+        description="Enables the authentication subsystem",
+    )
@@
-    workers: PositiveInt = Field(
-        1,
-        title="Number of workers",
-        description="Number of workers to be started",
-    )
+    workers: PositiveInt = Field(
+        1,
+        title="Number of workers",
+        description="Number of Uvicorn worker processes to start",
+    )

Feel free to ignore if you prefer the current phrasing; the existing text is already understandable.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac99077 and b873301.

📒 Files selected for processing (1)
  • src/models/config.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
⏰ 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). (4)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)
🔇 Additional comments (1)
src/models/config.py (1)

149-156: InMemoryCacheConfig.max_entries documentation looks good and consistent

The new Field metadata is clear, fixes the earlier typo, and uses “in‑memory cache” terminology that matches the class name. No further changes needed here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/models/config.py (3)

331-337: Consider enhancing the description for clarity.

The title and description are identical ("TLS configuration"). Consider making the description more informative, similar to how other fields provide additional context.

Example:

     tls_config: TLSConfiguration = Field(
         default_factory=lambda: TLSConfiguration(
             tls_certificate_path=None, tls_key_path=None, tls_key_password=None
         ),
         title="TLS configuration",
-        description="TLS configuration",
+        description="Transport Layer Security configuration for HTTPS support",
     )

339-348: Consider enhancing the description for clarity.

The title and description are identical ("CORS configuration"). Consider making the description more informative to match the pattern used in other fields.

Example:

     cors: CORSConfiguration = Field(
         default_factory=lambda: CORSConfiguration(
             allow_origins=["*"],
             allow_credentials=False,
             allow_methods=["*"],
             allow_headers=["*"],
         ),
         title="CORS configuration",
-        description="CORS configuration",
+        description="Cross-Origin Resource Sharing configuration for cross-domain requests",
     )

350-356: Consider extracting port validation to reduce duplication.

The port validation logic is duplicated between ServiceConfiguration (lines 350-356) and PostgreSQLDatabaseConfiguration (lines 228-233). Both validators check the same constraint with identical error messages.

Consider extracting this validation into a reusable function or Pydantic field validator to follow the DRY principle:

def validate_port_range(port: PositiveInt) -> PositiveInt:
    """Validate that port is within valid range (1-65535)."""
    if port > 65535:
        raise ValueError("Port value should be less than 65536")
    return port

Then use it in both classes:

 @model_validator(mode="after")
 def check_service_configuration(self) -> Self:
     """Check service configuration."""
-    if self.port > 65535:
-        raise ValueError("Port value should be less than 65536")
+    self.port = validate_port_range(self.port)
     return self
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b873301 and c4e4da2.

📒 Files selected for processing (1)
  • src/models/config.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
⏰ 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). (4)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: build-pr
🔇 Additional comments (3)
src/models/config.py (3)

152-156: LGTM!

The Field metadata is correctly structured with appropriate title and description. The typo mentioned in the previous review has been addressed.


287-293: LGTM!

The expanded docstring provides clear context about the service configuration and follows Google Python docstring conventions.


295-329: LGTM!

The Field declarations are well-structured with appropriate titles, descriptions, and default values. All type annotations are complete and follow coding guidelines.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/models/config.py (1)

152-156: InMemoryCacheConfig.max_entries Field metadata looks good

Type, default, and description are consistent with the class purpose and existing configuration patterns (extends ConfigurationBase, uses PositiveInt). No changes needed.

🧹 Nitpick comments (2)
src/models/config.py (2)

287-293: Minor wording tweaks in ServiceConfiguration docstring and workers description

The content is clear, but there’s a small grammar nit around “workers processes”:

-    """Service configuration.
-
-    Lightspeed Core Stack is a REST API service that accepts requests
-    on a specified hostname and port. It is also possible to enable
-    authentication and specify the number of Uvicorn workers. When more
-    workers are specified, the service can handle requests concurrently.
-    """
+    """Service configuration.
+
+    Lightspeed Core Stack is a REST API service that accepts requests
+    on a specified hostname and port. It is also possible to enable
+    authentication and specify the number of Uvicorn worker processes. When more
+    workers are specified, the service can handle requests concurrently.
+    """
...
-        description="Number of Uvicorn workers processes to start",
+        description="Number of Uvicorn worker processes to start",

This keeps the meaning while using more idiomatic English.

Also applies to: 313-317


359-372: Strong MCP server documentation; optional phrasing polish

The expanded ModelContextProtocolServer docstring and Field metadata are clear and useful. One small English style improvement you might consider:

-    url: str = Field(
-        ...,
-        title="MCP server URL",
-        description="URL to MCP server",
-    )
+    url: str = Field(
+        ...,
+        title="MCP server URL",
+        description="URL of the MCP server",
+    )

This is purely cosmetic; feel free to keep as-is if you prefer the current wording.

Also applies to: 374-390

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4e4da2 and 054bd02.

📒 Files selected for processing (1)
  • src/models/config.py (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : Use type hints `Optional[FilePath]`, `PositiveInt`, `SecretStr` for Pydantic configuration models
⏰ 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). (4)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)
🔇 Additional comments (1)
src/models/config.py (1)

335-337: TLS and CORS Field metadata are consistent and helpful

The added title and description for tls_config and cors align well with their respective configuration classes and should improve generated docs/UX. No functional or style issues spotted.

Also applies to: 345-347

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
src/models/config.py (4)

286-329: Minor wording tweaks in ServiceConfiguration field descriptions

The added metadata is useful; a few small grammar improvements would make the descriptions read more naturally:

     workers: PositiveInt = Field(
         1,
         title="Number of workers",
-        description="Number of Uvicorn workers processes to start",
+        description="Number of Uvicorn worker processes to start",
     )

     color_log: bool = Field(
         True,
         title="Color log",
-        description="Enables colorized log",
+        description="Enables colorized logging",
     )

     access_log: bool = Field(
         True,
         title="Access log",
-        description="Enables logging all access information",
+        description="Enables logging of all access information",
     )

Everything else in this block looks consistent and accurate.

Also applies to: 331-348


394-431: Clarify conflicting URL vs. library-mode configuration in LlamaStackConfiguration

The validator enforces sensible rules, but there’s one edge case where configuration can be contradictory:

  • If url is set and use_as_library_client is also True, the model currently passes validation and then treats the config as library mode (ignoring the URL), even though the url docstring says it is “used when library mode is disabled.”

To match the documentation and prevent confusing configs, consider rejecting this combination:

     if self.use_as_library_client:
-        # when use_as_library_client is set to true, Llama Stack will be run in library mode
-        # it means that library_client_config_path attribute must be set and must point to
-        # a regular readable YAML file
+        # when use_as_library_client is set to true, Llama Stack will be run in library mode
+        # it means that:
+        # - Llama Stack URL should not be set, and
+        # - library_client_config_path attribute must be set and must point to
+        #   a regular readable YAML file
+        if self.url is not None:
+            raise ValueError(
+                "Llama stack URL should not be set when library client mode is enabled"
+            )

The rest of the validator and the new field metadata look good.

Also applies to: 448-450, 459-462, 464-467


524-555: JwtRoleRule validation and metadata look strong; tiny wording polish

The split validators for JSONPath syntax, role list sanity, and regex patterns give very clear failure modes and should catch most misconfigurations early.

If you want to tighten the descriptions a bit:

     negate: bool = Field(
         False,
         title="Negate rule",
-        description="If set to true, the meaning of rule is negated",
+        description="If set to true, the meaning of the rule is negated",
     )
@@
     roles: list[str] = Field(
         ...,
         title="List of roles",
-        description="Roles to be assigned if rule matches",
+        description="Roles to be assigned if the rule matches",
     )

Functionally this block looks solid as-is.

Also applies to: 557-600


876-895: Improve QuotaLimiterConfiguration doc wording and align scheduler with ConfigurationBase

The new quota limiter description is helpful but has a few awkward phrases; rewording will make it easier to consume:

 class QuotaLimiterConfiguration(ConfigurationBase):
-    """Configuration for one quota limiter.
-
-    There are three configuration options for each limiter:
-
-    1. `period` specified in a human-readable form, see
-       https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT
-       for all possible options. When the end of the period is reached, quota is
-       reset or increased
-    2. `initial_quota` is set at beginning of the period
-    3. `quota_increase` this value (if specified) is used to increase quota
-       when period is reached
-
-    There are two basic use cases:
-
-    1. When quota needs to be reset specific value periodically (for example on
-       weekly on monthly basis), specify `initial_quota` to the required value
-    2. When quota needs to be increased by specific value periodically (for
-       example on daily basis), specify `quota_increase`
-    """
+    """Configuration for one quota limiter.
+
+    There are three configuration options for each limiter:
+
+    1. ``period`` is specified in a human-readable form, see
+       https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT
+       for all possible options. When the end of the period is reached, the
+       quota is reset or increased.
+    2. ``initial_quota`` is the value set at the beginning of the period.
+    3. ``quota_increase`` is the value (if specified) used to increase the
+       quota when the period is reached.
+
+    There are two basic use cases:
+
+    1. When the quota needs to be reset to a specific value periodically (for
+       example on a weekly or monthly basis), set ``initial_quota`` to the
+       required value.
+    2. When the quota needs to be increased by a specific value periodically
+       (for example on a daily basis), set ``quota_increase``.
+    """

Separately, QuotaSchedulerConfiguration is the only configuration-type model here that still inherits directly from BaseModel. To keep the “extra='forbid' on all config models” invariant, consider switching it to ConfigurationBase:

-class QuotaSchedulerConfiguration(BaseModel):
+class QuotaSchedulerConfiguration(ConfigurationBase):

This will cause unknown fields under scheduler: to be rejected instead of silently ignored, making it consistent with other configuration sections. Based on learnings, this keeps all configuration models aligned on ConfigurationBase + extra="forbid".

Also applies to: 897-925, 928-933

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 054bd02 and 28a9da1.

📒 Files selected for processing (1)
  • src/models/config.py (8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/**/*.py : Use `field_validator` and `model_validator` for custom validation in Pydantic models
⏰ 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). (4)
  • GitHub Check: build-pr
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: e2e_tests (ci)
  • GitHub Check: e2e_tests (azure)
🔇 Additional comments (3)
src/models/config.py (3)

149-156: InMemoryCacheConfig.max_entries Field metadata looks good

The new Field metadata is clear, matches surrounding configuration style, and fixes the previous typo/terminology issue. No changes needed here.


359-390: ModelContextProtocolServer documentation and metadata are clear

The expanded docstring plus Field metadata for name, provider_id, and url nicely describe how MCP servers are configured and will surface well in generated docs or UIs. I don’t see any behavioral or consistency issues in this block.


822-824: Conversation history docstring change is appropriate

Updating the class docstring from “cache” to “history” better reflects the broader responsibility of ConversationHistoryConfiguration while keeping the existing field names and validator behavior intact. No further changes needed here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/models/config.py (2)

1002-1004: Fix type error in default_factory for quota_handlers field.

The pipeline failures indicate that default_factory requires a callable, not a class type. While passing the class directly works at runtime, type checkers (mypy and pyright) correctly flag this as a type error.

Apply this diff to fix the type error:

     quota_handlers: QuotaHandlersConfiguration = Field(
-        default_factory=QuotaHandlersConfiguration
+        default_factory=lambda: QuotaHandlersConfiguration()
     )

This matches the pattern used for other complex default factories in the same class (e.g., line 989 for database and line 969 in QuotaHandlersConfiguration.scheduler).


1-1009: Module exceeds recommended line count.

The file has grown to 1009 lines, exceeding the pylint limit of 1000 lines per module. While the additions in this PR are legitimate documentation improvements, consider splitting this module in a future refactor to improve maintainability. For example:

  • Move authentication-related models to src/models/auth_config.py
  • Move database-related models to src/models/db_config.py
  • Move quota-related models to src/models/quota_config.py

For now, you can suppress the warning by adding # pylint: disable=too-many-lines at the top of the file if you prefer to defer the refactoring.

🧹 Nitpick comments (2)
src/models/config.py (2)

427-431: Consider using FilePath type hint for consistency.

The library_client_config_path field is currently typed as Optional[str], but according to the coding guidelines and patterns used elsewhere in this file (e.g., TLSConfiguration.tls_certificate_path), configuration file paths should use the FilePath type hint. This provides automatic validation at parse time and is more consistent with the established pattern.

Based on learnings, configuration models should use Optional[FilePath] for file paths.

Apply this diff:

-    library_client_config_path: Optional[str] = Field(
+    library_client_config_path: Optional[FilePath] = Field(
         None,
         title="Llama Stack configuration path",
         description="Path to configuration file used when Llama Stack is run in library mode",
     )

If you use FilePath, you may need to adjust the validator at line 475-477 since Pydantic will have already validated file existence. Consider whether the additional checks in checks.file_check (readability, etc.) are still needed.


588-602: Minor: Improve error message clarity.

The error message at lines 593-595 could be slightly clearer by adding "got" before the type name.

Apply this diff:

                 raise ValueError(
-                    f"MATCH operator requires a string pattern, {type(self.value).__name__}"
+                    f"MATCH operator requires a string pattern, got {type(self.value).__name__}"
                 )

Otherwise, the validation logic is comprehensive and well-structured, covering JSONPath validity, role constraints, and regex pattern validation appropriately.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9f4b66 and cb40f15.

📒 Files selected for processing (1)
  • src/models/config.py (8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/**/*.py : Pydantic configuration classes must extend `ConfigurationBase`; data models must extend `BaseModel`
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : Use type hints `Optional[FilePath]`, `PositiveInt`, `SecretStr` for Pydantic configuration models
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields

Applied to files:

  • src/models/config.py
🪛 GitHub Actions: Pyright
src/models/config.py

[error] 1003-1003: uv run pyright src failed: Argument of type "type[QuotaHandlersConfiguration]" cannot be assigned to parameter "default_factory" of type "(() -> _T@Field) | ((dict[str, Any]) -> _T@Field)" in function "Field". Type "type[QuotaHandlersConfiguration]" is not assignable to type "(() -> _T@Field) | ((dict[str, Any]) -> _T@Field)". (pyright: reportArgumentType)

🪛 GitHub Actions: Python linter
src/models/config.py

[error] 1-1009: Pylint: C0302 Too many lines in module (1009/1000) (too-many-lines).

🪛 GitHub Actions: Type checks
src/models/config.py

[error] 1003-1003: mypy: Argument 'default_factory' to 'Field' has incompatible type 'type[QuotaHandlersConfiguration]'; expected 'Callable[[], Never] | Callable[[dict[str, Any]], Never]'.

⏰ 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). (4)
  • GitHub Check: build-pr
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)
🔇 Additional comments (7)
src/models/config.py (7)

152-156: LGTM!

The Field metadata is properly structured with clear title and description. The typo mentioned in the previous review has been addressed.


286-356: LGTM!

The Field metadata additions are well-structured with clear titles and descriptions. The service configuration is now more self-documenting. The port validation correctly enforces the TCP port range limit.


358-391: LGTM!

The expanded docstring provides excellent context about MCP servers with helpful resource links. The Field metadata is clear and well-structured. Note that while the name field description mentions uniqueness requirement, this constraint is likely enforced at the application level rather than at the model level.


825-825: LGTM!

The docstring update aligns the terminology with the class name, improving consistency.


878-929: LGTM!

The expanded docstring provides excellent context about quota limiter configuration with clear use case examples. The Field metadata is well-structured with appropriate type hints (NonNegativeInt, Literal).


931-938: LGTM!

The Field metadata is clear and appropriate, with a proper default value and descriptive text.


941-978: LGTM!

The expanded docstring and Field metadata additions are well-structured and clear. The default_factory usage for the scheduler field correctly uses a lambda to instantiate the configuration object.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/models/config.py (1)

395-480: Tighten Llama Stack config validation when both url and use_as_library_client=True are set

The docstring/comments say that in library mode the Llama Stack URL “should not be set”, and the url description says it’s used when library mode is disabled. Currently, check_llama_stack_model only validates the case where url is None; a config with both url set and use_as_library_client=True passes validation.

To avoid ambiguous configs, consider rejecting that combination, e.g. inside the if self.use_as_library_client: block:

     if self.use_as_library_client:
-        # when use_as_library_client is set to true, Llama Stack will be run in library mode
-        # it means that:
-        # - Llama Stack URL should not be set, and
-        # - library_client_config_path attribute must be set and must point to
-        #   a regular readable YAML file
+        # when use_as_library_client is set to true, Llama Stack will be run in library mode
+        # it means that:
+        # - Llama Stack URL should not be set, and
+        # - library_client_config_path attribute must be set and must point to
+        #   a regular readable YAML file
+        if self.url is not None:
+            raise ValueError(
+                "Llama stack library client mode is enabled but a URL is also configured; "
+                "unset url when using library mode"
+            )
docs/config.md (2)

97-118: Polish user_data_collection field description for clarity

The description is readable but has a small spacing/wording issue:

  • “user data(transcription history and feedbacks)” → missing space before the parenthesis and “feedbacks” is uncommon.

Consider:

-| user_data_collection |  | This section contains configuration for subsystem that collects user data(transcription history and feedbacks). |
+| user_data_collection |  | This section contains configuration for a subsystem that collects user data (transcription history and feedback). |

323-374: Tidy quota-related wording (grammar and hyphenation)

Minor English fixes to make the QuotaHandlers/QuotaLimiter docs read more naturally:

  • “which is name that makes sense” → “which is a name that makes sense”.
  • “human readable” → “human-readable”.

For example:

-It is possible to limit quota usage per user or per service or services
-(that typically run in one cluster). Each limit is configured as a separate
-_quota limiter_. It can be of type `user_limiter` or `cluster_limiter`
-(which is name that makes sense in OpenShift deployment).
+It is possible to limit quota usage per user or per service or for services
+(that typically run in one cluster). Each limit is configured as a separate
+_quota limiter_. It can be of type `user_limiter` or `cluster_limiter`
+(which is a name that makes sense in OpenShift deployment).
...
-| type | string | Quota limiter type, either user_limiter or cluster_limiter |
-| name | string | Human readable quota limiter name |
+| type | string | Quota limiter type, either user_limiter or cluster_limiter |
+| name | string | Human-readable quota limiter name |
...
-| period | string | Period specified in human readable form |
+| period | string | Period specified in human-readable form |
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb40f15 and 2db1740.

📒 Files selected for processing (4)
  • docs/config.html (15 hunks)
  • docs/config.json (26 hunks)
  • docs/config.md (7 hunks)
  • src/models/config.py (9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.py: Use absolute imports for internal modules in LCS project (e.g., from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Use logger = logging.getLogger(__name__) pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) and Optional[Type] or Type | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Use async def for I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes: Configuration for config classes, Error/Exception for exceptions, Resolver for strategy patterns, Interface for abstract base classes
Abstract classes must use ABC with @abstractmethod decorators
Include complete type annotations for all class attributes in Python classes
Use import logging and module logger pattern with standard log levels: debug, info, warning, error

Files:

  • src/models/config.py
src/models/config.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/config.py: All configuration must use Pydantic models extending ConfigurationBase with extra="forbid" to reject unknown fields
Use type hints Optional[FilePath], PositiveInt, SecretStr for Pydantic configuration models

Files:

  • src/models/config.py
src/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/models/**/*.py: Use @field_validator and @model_validator for custom validation in Pydantic models
Pydantic configuration classes must extend ConfigurationBase; data models must extend BaseModel

Files:

  • src/models/config.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/**/*.py : Pydantic configuration classes must extend `ConfigurationBase`; data models must extend `BaseModel`
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : Use type hints `Optional[FilePath]`, `PositiveInt`, `SecretStr` for Pydantic configuration models
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/**/*.py : Use `field_validator` and `model_validator` for custom validation in Pydantic models
📚 Learning: 2025-11-24T16:58:04.410Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:58:04.410Z
Learning: Applies to src/models/config.py : All configuration must use Pydantic models extending `ConfigurationBase` with `extra="forbid"` to reject unknown fields

Applied to files:

  • src/models/config.py
🧬 Code graph analysis (1)
src/models/config.py (2)
src/configuration.py (4)
  • mcp_servers (101-105)
  • customization (127-131)
  • inference (134-138)
  • conversation_cache (162-170)
src/cache/cache_factory.py (1)
  • conversation_cache (20-49)
🪛 LanguageTool
docs/config.md

[style] ~110-~110: Consider using “inaccessible” to avoid wordiness.
Context: ...figured in the llama-stack run.yaml are not accessible to lightspeed-core agents. | | authenti...

(NOT_ABLE_PREMIUM)


[style] ~278-~278: Consider using “inaccessible” to avoid wordiness.
Context: ...figured in the llama-stack run.yaml are not accessible to lightspeed-core agents. Useful reso...

(NOT_ABLE_PREMIUM)


[style] ~364-~364: ‘on a daily basis’ might be wordy. Consider a shorter alternative.
Context: ...ific value periodically (for example on a daily basis), set quota_increase. | Field | T...

(EN_WORDINESS_PREMIUM_ON_A_DAILY_BASIS)


[grammar] ~370-~370: Use a hyphen to join words.
Context: ...luster_limiter | | name | string | Human readable quota limiter name | | initial_...

(QB_NEW_EN_HYPHEN)


[grammar] ~373-~373: Use a hyphen to join words.
Context: ...iod | string | Period specified in human readable form | ## QuotaSchedulerConfi...

(QB_NEW_EN_HYPHEN)

🪛 markdownlint-cli2 (0.18.1)
docs/config.md

256-256: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


257-257: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


258-258: Unordered list indentation
Expected: 0; Actual: 2

(MD007, ul-indent)


351-351: Bare URL used

(MD034, no-bare-urls)

⏰ 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). (4)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests (azure)
  • GitHub Check: e2e_tests (ci)
🔇 Additional comments (1)
docs/config.json (1)

150-155: Review comment cannot be verified—repository access failed. However, web search confirms that in Pydantic v2, PositiveInt generates "exclusiveMinimum": 0 (not "minimum": 0 as claimed), which correctly enforces values > 0 at both documentation and runtime. If the JSON schema in docs/config.json actually shows "minimum": 0 for these fields, the issue is manual schema misconfiguration, not a Pydantic mismatch. The listed fields and their actual type declarations cannot be confirmed without codebase access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant